From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 2/5] drm/i915: Fix up fifo underrun tracking, take N Date: Mon, 26 May 2014 11:09:30 +0300 Message-ID: <20140526080930.GD27580@intel.com> References: <1400774195-19414-1-git-send-email-daniel.vetter@ffwll.ch> <1400774195-19414-2-git-send-email-daniel.vetter@ffwll.ch> <20140522165552.GX27580@intel.com> <20140522201033.GB14357@phenom.ffwll.local> <20140523081134.GC27580@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 98A586E209 for ; Mon, 26 May 2014 01:09:34 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Fri, May 23, 2014 at 10:21:24AM +0200, Daniel Vetter wrote: > On Fri, May 23, 2014 at 10:11 AM, Ville Syrj=E4l=E4 > wrote: > >> For enabled->enabled I think that can happen in crtc_enable - we > >> unconditionally enable underrun reporting againg to clear out old fail= (or > >> firmware setups). But we don't always disable it when disabling the cr= tc > >> since some platforms/ports don't underrun when disabled. > > > > Hmm. Actually since we don't necessarily notice the underruns with the > > shared error interrupt, maybe we need to also throw an explicit underrun > > check at the end of crtc_disable. That would mean we'd catch the underr= un > > at the very latest there, and crtc_enable can then clear the bit without > > worrying about losing valid underrun reports. > > > > So, I think this patch looks OK. But we will need to keep this issue in > > mind if we add the underrun report re-enable timer, or something like i= t. > > Since my quick hack for that just blindly walks the crtcs and > > (re)enables the underrun reporting for everything. > = > Hm yeah, we might want a fifo underrun check like on gmch platforms. > Otoh if we disable the shared error interrupt things are already > rather bad, and the problem on the first pipe should have accurate > irq-based reporting. The 2nd pipe is hidden, but should show up as > soon as the first issue is addressed. > = > So I don't think we really should worry about this. Maybe more > intersting would be to check the _other_ pipes when we re-enable fifo > underruns. At least some of the reports we've seen suggest that > modesets on the pch influence each another ... But we enable underrun reporting before doing the modeset, so at that time the modeset can't have caused underruns on any pipe. So adding the explicit check to the end .crtc_enable() (as I did for gmch) should catch those nicely. -- = Ville Syrj=E4l=E4 Intel OTC