Re: [PATCH] drm/i915: Revert async unpin and nonblocking atomic commit

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, May 25, 2016 at 08:57:49AM +0200, Maarten Lankhorst wrote:
> 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@linux.intel.com>

Ok applied&pushed with irc acks from Dave, Jani&Ville added.

> 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..

Yeah, this entire story would have been much less tricky without that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx