From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 00/12] irq vblank handling rework Date: Wed, 21 May 2014 10:35:59 +0200 Message-ID: <20140521083559.GI24095@phenom.ffwll.local> References: <1400093477-3217-1-git-send-email-daniel.vetter@ffwll.ch> <20140521082655.GK27580@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-ee0-f51.google.com (mail-ee0-f51.google.com [74.125.83.51]) by gabe.freedesktop.org (Postfix) with ESMTP id 12B686E222 for ; Wed, 21 May 2014 01:36:03 -0700 (PDT) Received: by mail-ee0-f51.google.com with SMTP id e51so1306842eek.38 for ; Wed, 21 May 2014 01:36:03 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140521082655.GK27580@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Cc: Daniel Vetter , Intel Graphics Development , DRI Development List-Id: dri-devel@lists.freedesktop.org On Wed, May 21, 2014 at 11:26:55AM +0300, Ville Syrj=E4l=E4 wrote: > For everything but the reset_vblank_counter() thing: > Reviewed-by: Ville Syrj=E4l=E4 > = > And another caveat: I only glanced at the crtc_helper stuff. It looks > sane but I didn't go reading through the drivers to figure out if they > would fall over or work. Oh, the rfc was really just that. I don't have any plans to burn my hands trying to merge those patches ;-) Especially since interest from non-i915 hackers seems to be low. > About the reset_vblank_counter(), I think we might still need it to keep > the counter sane when the power well goes off. But I think we have > other problems on that front esp. with suspend to disk. There the counter > should definitely get reset on all platforms, so we migth apply any kind > of diff to the user visible value. The fix would likely be to skip the > diff adjustment when resuming. > = > I tried to take a quick look at that yesterday but there was something > really fishy happening as the code didn't seem to observe the wraparound > at all, even though I confirmed w/ intel_reg_read that it definitely > did appear to wrap. I'll take another look at it today. > = > Another idea might be to rip out the diff adjustment altogether. That > should just mean that the user visible counter wouldn't advance at all > between drm_vblank_off() and drm_vblank_on(). But that might actually > be the sane thing to do. Maybe we should just do a +1 there to make > sure we don't report the same value before and after modeset. It should > fix both the suspend problems and the power well problem. Hm, like I've mentioned yesterday on irc the tests I have actually pass, at least if I throw your sanitize_crtc patch on top. vblank frame counter values monotonically increase across suspend/resume, runtime pm and anything else I manged to throw at it. And the limit in the test is 100 frames later, but I've only observed a few tens at most. So I think the code as-is actually works. Whether intentional or not is still under dispute though ;-) The real problem I have with the hsw counter reset is that the same issue should affect _any_ platform where we support runtime pm. Like snb or byt. But the code isn't there. Also if we have such a bug then it will also affect hibernate and suspend to disk. Which means that this should be done in drm_crtc_vblank_off/on, not in the guts of some random platforms runtime pm code. So in my opinion the hsw vblank_count reset code needs to go anyway because: - Either it isn't need any more (and we have the tests for this now) and it's just cargo-culted duct-tape. - Or we need, but then it's in the wrong spot. Given that can you reconsider that patch please? Thanks, Daniel > = > On Wed, May 14, 2014 at 08:51:02PM +0200, Daniel Vetter wrote: > > Hi all, > > = > > This is Ville's vblank rework series, slightly pimped. I've added kerne= ldoc, > > some polish and also some additional nasty igt testcases for these crtc= on/off > > vs vblank issues. Seems all to hold together nicely. > > = > > Now one thing I wanted to do is roll this out across all drm drivers, b= ut that > > looks ugly: Many drivers don't support vblanks really, and I couldn't f= ully > > audit whether they'd e.g. blow up when userspace tries to use the vblan= k wait > > ioctl. But I think we want to have more sanity since otherwise userspac= e is > > doomed to carry countless ugly hacks around forever. > > = > > The last two patches are my attempt at that. By doing the drm_vblank_on= /off > > dance in the crtc helpers after we know that the crtc is on/off we shou= ld have > > at least a somewhat sane baseline behaviour. Note that since drm_vblank= _on/off > > are idempotent drivers are free to call these functions in their callba= ck at an > > earlier time, where it makes more sense. But at least it's guaranteed t= o happen. > > = > > Otoh drivers still need to manually complete pageflips, so this doesn't= solve > > all the issues. But until we have a solid cross-driver testsuite for th= ese > > corner-cases (all the interesting stuff happens when you race vblank wa= its > > against concurrent modesets, dpms, or system/runtime suspend/resume) we= 're > > pretty much guaranteed to have some that works at best occasionally and= is > > guaranteed to have different behaviour in corner cases. Note that the p= atches > > won't degrade behaviour for existing drivers, the drm core changes simp= ly allow > > us to finally fix things up correctly. > > = > > I guess in a way this is a more generic discussion for the drm subsyste= m at > > large. > > = > > Coments and review highly welcome. > > = > > Cheers, Daniel > > = > > Daniel Vetter (8): > > drm/i915: Remove drm_vblank_pre/post_modeset calls > > drm/doc: Discourage usage of MODESET_CTL ioctl > > drm/irq: kerneldoc polish > > drm/irq: Add kms-native crtc interface functions > > drm/i915: rip our vblank reset hacks for runtime PM > > drm/i915: Accurately initialize fifo underrun state on gmch platforms > > [RFC] drm/irq: More robustness in drm_vblank_on|off > > [RFC] drm/crtc-helper: Enforce sane vblank counter semantics > > = > > Peter Hurley (1): > > drm: Use correct spinlock flavor in drm_vblank_get() > > = > > Ville Syrj=E4l=E4 (3): > > drm: Make the vblank disable timer per-crtc > > drm: Make blocking vblank wait return when the vblank interrupts get > > disabled > > drm: Add drm_vblank_on() > > = > > Documentation/DocBook/drm.tmpl | 16 +- > > drivers/gpu/drm/drm_crtc_helper.c | 12 ++ > > drivers/gpu/drm/drm_irq.c | 377 ++++++++++++++++++++++++++-= -------- > > drivers/gpu/drm/i915/i915_irq.c | 4 +- > > drivers/gpu/drm/i915/intel_display.c | 36 ++-- > > drivers/gpu/drm/i915/intel_drv.h | 2 - > > drivers/gpu/drm/i915/intel_pm.c | 40 ---- > > include/drm/drmP.h | 10 +- > > 8 files changed, 341 insertions(+), 156 deletions(-) > > = > > -- = > > 1.8.3.1 > > = > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > = > -- = > Ville Syrj=E4l=E4 > Intel OTC -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch