Also seen on https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_7/fi-blb-e6850/igt@kms_rmfb@rmfb-ioctl.html

https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_7/fi-bwr-2160/igt@kms_rmfb@rmfb-ioctl.html

(kms_rmfb:1491) CRITICAL: Test assertion failure function test_rmfb, file ../tests/kms_rmfb.c:120:
(kms_rmfb:1491) CRITICAL: Failed assertion: planeres->fb_id == 0
(kms_rmfb:1491) CRITICAL: error: 46 != 0
Subtest rmfb-ioctl failed.

Seem like on pre-ILK rmfb does not behave like it should.

The test looks correct and does what it advertises:
/*
 * 1. Set primary plane to a known fb.
 * 2. Make sure getcrtc returns the correct fb id.
 * 3. Call rmfb on the fb.
 * 4. Make sure getcrtc returns 0 fb id.
 *
 * RMFB is supposed to free the framebuffers from any and all planes,
 * so test this and make sure it works.
 */

The first thing that can go wrong is race condition - freeing frambuffers from the planes is a deferred task:
  /*
   * we now own the reference that was stored in the fbs list
   *
   * drm_framebuffer_remove may fail with -EINTR on pending signals,
   * so run this in a separate stack as there's no way to correctly
   * handle this after the fb is already removed from the lookup table.
   */
  struct drm_mode_rmfb_work arg;

  INIT_WORK_ONSTACK(&arg.work, drm_mode_rmfb_work_fn);
  INIT_LIST_HEAD(&arg.fbs);
  list_add_tail(&fb->filp_head, &arg.fbs);

  schedule_work(&arg.work);
  flush_work(&arg.work);
  destroy_work_on_stack(&arg.work);

But there are also two paths there - for atomic and legacy removing of fb:
  if (drm_drv_uses_atomic_modeset(dev)) {
  	int ret = atomic_remove_fb(fb);
  	WARN(ret, "atomic remove_fb failed with %i\n", ret);
  } else
  	legacy_remove_fb(fb);
  }

And the offending machines seem to exactly match the legacy path:
  /* Disable nuclear pageflip by default on pre-ILK */
  if (!i915_modparams.nuclear_pageflip && match_info->gen < 5)
  	dev_priv->drm.driver_features &= ~DRIVER_ATOMIC;


The code on the legacy path looks correct though, and the main difference seem to be:
drm_modeset_lock_all(dev);

Which may take time to acquire and it's actually combination of the two possible issues from above.


Possible user impact in worst case: memory leak when using rmfb for pre-atomic platforms, but quite likely it's not that and there were no reports of this in the wild. Keeping the priority as "medium".

Let's find out whether it's just the delay:
https://patchwork.freedesktop.org/series/65694/

https://intel-gfx-ci.01.org/tree/drm-tip/TrybotIGT_36/fi-elk-e7500/igt@kms_rmfb@close-fd.html

The test still fails on non-atomic platforms, even with the 5s 'wait_on'. I think we actually have an issue with the legacy codepath not releasing the FB correctly. This is beyond being debugable over Trybot.

https://patchwork.freedesktop.org/patch/326841/?series=65790&rev=1

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/drm/intel/issues/107.