[Intel-gfx] [PATCH] drm/i915: Revert async unpin and nonblocking atomic commit

[Maarten Lankhorst]

Op 24-05-16 om 17:49 schreef Daniel Vetter:
> This reverts the following patches:
>
> d55dbd06bb5e1399aba9ab5227465339d1bbefff drm/i915: Allow nonblocking update of pageflips.
> 15c86bdb760185e871c7a0f559978328aa500971 drm/i915: Check for unpin correctness.
> 95c2ccdc82d520f59ae3b6fdc097b63c9b7082bb Reapply "drm/i915: Avoid stalling on pending flips for legacy cursor updates"
> a6747b7304a9d66758a196d885dab8bbfa5e7d1f drm/i915: Make unpin async.
> 03f476e1fcb42fca88fc50b94b0d3adbdbe887f0 drm/i915: Prepare connectors for nonblocking checks.
> 2099deffef4404f949ba1b68d2b17e0608190bc2 drm/i915: Pass atomic states to fbc update functions.
> ee7171af72c39c18b7d7571419a4ac6ca30aea66 drm/i915: Remove reset_counter from intel_crtc.
> 2ee004f7c59b2e642f0bb2834f847d756f2dd7b7 drm/i915: Remove queue_flip pointer.
> b8d2afae557dbb9b9c7bc6f6ec4f5278f3c4c34e drm/i915: Remove use_mmio_flip kernel parameter.
> 8dd634d922615ec3a9af7976029110ec037f8b50 drm/i915: Remove cs based page flip support.
> 143f73b3bf48c089b40f58462dd7f7c199fd4f0f drm/i915: Rework intel_crtc_page_flip to be almost atomic, v3.
> 84fc494b64e8c591be446a966b7447a9db519c88 drm/i915: Add the exclusive fence to plane_state.
> 6885843ae164e11f6c802209d06921e678a3f3f3 drm/i915: Convert flip_work to a list.
> aa420ddd8eeaa5df579894a412289e4d07c2fee9 drm/i915: Allow mmio updates on all platforms, v2.
> afee4d8707ab1f21b7668de995be3a5961e83582 Revert "drm/i915: Avoid stalling on pending flips for legacy cursor updates"
>
> "drm/i915: Allow nonblocking update of pageflips" should have been
> split up, misses a proper commit message and seems to cause issues in
> the legacy page_flip path as demonstrated by kms_flip.
>
> "drm/i915: Make unpin async" doesn't handle the unthrottled cursor
> updates correctly, leading to an apparent pin count leak. This is
> caught by the WARN_ON in i915_gem_object_do_pin which screams if we
> have more than DRM_I915_GEM_OBJECT_MAX_PIN_COUNT pins.
>
> Unfortuantely we can't just revert these two because this patch series
> came with a built-in bisect breakage in the form of temporarily
> removing the unthrottled cursor update hack for legacy cursor ioctl.
> Therefore there's no other option than to revert the entire pile :(
>
> There's one tiny conflict in intel_drv.h due to other patches, nothing
> serious.
>
> Normally I'd wait a bit longer with doing a maintainer revert, but
> since the minimal set of patches we need to revert (due to the bisect
> breakage) is so big, time is running out fast. And very soon
> (especially after a few attempts at fixing issues) it'll be really
> hard to revert things cleanly.
>
> Lessons learned:
> - Not a good idea to rush the review (done by someone fairly new to
>   the area) and not make sure domain experts had a chance to read it.
>
> - Patches should be properly split up. I only looked at the two
>   patches that should be reverted in detail, but both look like the
>   mix up different things in one patch.
>
> - Patches really should have proper commit messages. Especially when
>   doing more than one thing, and especially when touching critical and
>   tricky core code.
>
> - Building a patch series and r-b stamping it when it has a built-in
>   bisect breakage is not a good idea.
>
> - I also think we need to stop building up technical debt by
>   postponing atomic igt testcases even longer. I think it's clear that
>   there's enough corner cases in this beast that we really need to
>   have the testcases _before_ the next step lands.
Acked-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>

It should be possible to make avoid stalling work without much issue after reworking intel_crtc_page_flip to be almost atomic,
but it would be better to edit the mmio updates on all platforms patch for lesson #4..