From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 5/6] drm: Prevent vblank counter jumps with timestamp based update method. Date: Tue, 9 Feb 2016 16:03:47 +0100 Message-ID: <20160209150347.GZ11240@phenom.ffwll.local> References: <1454894009-15466-1-git-send-email-mario.kleiner.de@gmail.com> <1454894009-15466-6-git-send-email-mario.kleiner.de@gmail.com> <20160209100917.GP11240@phenom.ffwll.local> <56B9EF5A.6050902@gmail.com> <20160209141149.GP23290@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: <20160209141149.GP23290@intel.com> Sender: stable-owner@vger.kernel.org To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Cc: Mario Kleiner , Daniel Vetter , dri-devel@lists.freedesktop.org, linux@bernd-steinhauser.de, stable@vger.kernel.org, michel@daenzer.net, vbabka@suse.cz, daniel.vetter@ffwll.ch, alexander.deucher@amd.com, christian.koenig@amd.com List-Id: dri-devel@lists.freedesktop.org On Tue, Feb 09, 2016 at 04:11:49PM +0200, Ville Syrj=E4l=E4 wrote: > On Tue, Feb 09, 2016 at 02:53:30PM +0100, Mario Kleiner wrote: > > On 02/09/2016 11:09 AM, Daniel Vetter wrote: > > > On Mon, Feb 08, 2016 at 02:13:28AM +0100, Mario Kleiner wrote: > > >> The changes to drm_update_vblank_count() in Linux 4.4 added a > > >> method to emulate a hardware vblank counter by use of high > > >> precision vblank timestamps if a kms driver supports those, > > >> but doesn't suppport hw vblank counters. > > >> > > >> That method assumes that the old timestamp from a previous > > >> invocation is valid, but that is not always the case. E.g., > > >> if drm_reset_vblank_timestamp() gets called during drm_vblank_on= () > > >> or drm_update_vblank_count() gets called outside vblank irq and > > >> the high precision timestamping can't deliver a precise timestam= p, > > >> ie. drm_get_last_vbltimestamp() delivers a return value of false= , > > >> then those functions will initialize the old timestamp to zero > > >> to mark it as invalid. > > >> > > >> A following call to drm_update_vblank_count() would then calcula= te > > >> elapsed time with vblank irqs off as current vblank timestamp mi= nus > > >> the zero old timestamp and compute a software vblank counter inc= rement > > >> that corresponds to system uptime, causing a large forward jump = of the > > >> software vblank counter. That jump in turn can cause too long wa= its > > >> in drmWaitVblank and very long delays in delivery of vblank even= ts, > > >> resulting in hangs of userspace clients. > > >> > > >> This problem can be observed on nouveau-kms during machine > > >> suspend->resume cycles, where drm_vblank_off is called during > > >> suspend, drm_vblank_on is called during resume and the first > > >> queries to drm_get_last_vbltimestamp() don't deliver high > > >> precision timestamps, resulting in a large harmful counter jump. > > > > > > Why does nouveau enable vblank interrupts before it can get valid > > > timestamps? That sounds like the core bug here, and papering over= that in > > > the vblank code feels very wrong to me. Maybe we should instead j= ust not > > > sample the vblank at all if the timestamp fails and splat a big W= ARN_ON > > > about this, to give driver writers a chance to fix up their mess? > > > -Daniel > > > > >=20 > > The high precision timestamping is allowed to fail for a kms driver= =20 > > under some conditions which are not implementation errors of the dr= iver,=20 > > or getting disabled by user override, so we should handle that robu= stly.=20 > > We handle it robustly everywhere else in the drm, so we should do i= t=20 > > here as well. > >=20 > > E.g., nouveau generally supports timestamping/scanout position quer= ies,=20 > > but can't support it on old pre NV-50 hardware with some output typ= e=20 > > (either on analog VGA, or DVI-D, can't remember atm. which one is=20 > > unsupported). >=20 > I think the surprising thing here is that it fails first and then > succeeds on the same crtc/output combo presumably. Yeah exactly this. Failing consistently is ok imo (and probably should = be handled). Failing first and then later on working (without changing the mode config in between) seems suspicous. That shouldn't ever happen rea= lly and seems like a driver bug (like enabling vblanks too early, before th= e pipe is fully up&running). -Daniel >=20 > I guess in theory the driver could fail during random times for whate= ver > reason, though I tend to think that the driver should either make it > robust or not even pretend to support it. >=20 > I suppose one failure mode the driver can't really do anything about = is > some random SMI crap or something stalling the timestamp queries enou= gh > that we fail the precisions tests and exhaust the retry limits. So ye= ah, > making it robust against that kind of nastyness sounds line it might = be > a good idea. Though perhaps it should be something a bit more severe > than a DRM_DEBUG since I think it really shouldn't happen when the > driver and system are otherwise sane. Of course if it routinely fails > with some driver then simply making it spew errors into dmesg isn't > so nice, unless the driver also gets fixed. >=20 > >=20 > > There are also new Soc drivers showing up which support those metho= ds,=20 > > but at least i didn't verify or test if their implementations are g= ood=20 > > enough for the needs of the new drm_udpate_vblank_count() which is = more=20 > > sensitive to kms drivers being even slightly off. > >=20 > > -mario > >=20 > > >> > > >> Fix this by checking if the old timestamp used for this calculat= ions > > >> is zero =3D=3D invalid. If so, perform a counter increment of +1= to > > >> prevent large counter jumps and reinitialize the timestamps to > > >> sane values. > > >> > > >> Signed-off-by: Mario Kleiner > > >> Cc: # 4.4+ > > >> Cc: michel@daenzer.net > > >> Cc: vbabka@suse.cz > > >> Cc: ville.syrjala@linux.intel.com > > >> Cc: daniel.vetter@ffwll.ch > > >> Cc: dri-devel@lists.freedesktop.org > > >> Cc: alexander.deucher@amd.com > > >> Cc: christian.koenig@amd.com > > >> --- > > >> drivers/gpu/drm/drm_irq.c | 7 +++++++ > > >> 1 file changed, 7 insertions(+) > > >> > > >> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq= =2Ec > > >> index fb17c45..88bdf19 100644 > > >> --- a/drivers/gpu/drm/drm_irq.c > > >> +++ b/drivers/gpu/drm/drm_irq.c > > >> @@ -216,6 +216,13 @@ static void drm_update_vblank_count(struct = drm_device *dev, unsigned int pipe, > > >> DRM_DEBUG_VBL("crtc %u: Redundant vblirq ignored." > > >> " diff_ns =3D %lld, framedur_ns =3D %d)\n", > > >> pipe, (long long) diff_ns, framedur_ns); > > >> + > > >> + /* No valid t_old to calculate diff? Bump +1 to force reinit.= */ > > >> + if (t_old->tv_sec =3D=3D 0 && t_old->tv_usec =3D=3D 0) { > > >> + DRM_DEBUG_VBL("crtc %u: No baseline ts. Bump +1.\n", > > >> + pipe); > > >> + diff =3D 1; > > >> + } > > >> } else { > > >> /* some kind of default for drivers w/o accurate vbl timesta= mping */ > > >> diff =3D (flags & DRM_CALLED_FROM_VBLIRQ) !=3D 0; > > >> -- > > >> 1.9.1 > > >> > > > >=20 > --=20 > Ville Syrj=E4l=E4 > Intel OTC --=20 Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch