From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 5/5] drm/i915: Shuffle fifo underrun disable/enable points for gmch platforms Date: Wed, 21 May 2014 16:54:20 +0200 Message-ID: <20140521145420.GI14357@phenom.ffwll.local> References: <1400258425-32664-1-git-send-email-ville.syrjala@linux.intel.com> <1400258425-32664-6-git-send-email-ville.syrjala@linux.intel.com> <20140521145231.GH14357@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-ee0-f50.google.com (mail-ee0-f50.google.com [74.125.83.50]) by gabe.freedesktop.org (Postfix) with ESMTP id 7124D6EA5A for ; Wed, 21 May 2014 07:54:24 -0700 (PDT) Received: by mail-ee0-f50.google.com with SMTP id e51so1688461eek.37 for ; Wed, 21 May 2014 07:54:23 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140521145231.GH14357@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: ville.syrjala@linux.intel.com Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, May 21, 2014 at 04:52:31PM +0200, Daniel Vetter wrote: > On Fri, May 16, 2014 at 07:40:25PM +0300, ville.syrjala@linux.intel.com w= rote: > > From: Ville Syrj=E4l=E4 > > = > > Gen2 reports FIFO underruns whenever no planes are enabled on the pipe. > > So in order to avoid false positives we must enable the FIFO underrun > > reporting only when at least one plane is enabled on the pipe. For > > now just move the underrun reporting enable/disable points to the > > other side of the plane enable/disable point. That doesn't cover cases > > when we turn off all the planes for the pipe but leave the pipe running > > on purpose, but it's better than the current situation. > > = > > On gen4+ we can actually move the underrun reporting enable/disable to > > the opposite ends of the crtc enable/disable hooks. I suppose in theory > > we could leave the underrun reporting enabled all the time, except on > > VLV where PIPESTAT stops working when the display power well is down. > > If we ever get around to unifying the PIPESTAT irq handling for all > > gmch platforms, we should still follow the VLV route for other platform= s. > > It would also micro-optimize the irq handler a bit since we could then > > skip the PIPESTAT reads for all disabled pipes. > > = > > Gen3 is still a mystery, but for now I'm going to assume it behaves > > like gen4+. > > = > > Signed-off-by: Ville Syrj=E4l=E4 > > --- > > drivers/gpu/drm/i915/intel_display.c | 30 +++++++++++++++++++++++++++-= -- > > 1 file changed, 27 insertions(+), 3 deletions(-) > > = > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i91= 5/intel_display.c > > index 1b5164c..7f61047 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -4514,6 +4514,8 @@ static void valleyview_crtc_enable(struct drm_crt= c *crtc) > > = > > intel_crtc->active =3D true; > > = > > + intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); > > + > > for_each_encoder_on_crtc(dev, crtc, encoder) > > if (encoder->pre_pll_enable) > > encoder->pre_pll_enable(encoder); > > @@ -4537,7 +4539,6 @@ static void valleyview_crtc_enable(struct drm_crt= c *crtc) > > = > > intel_update_watermarks(crtc); > > intel_enable_pipe(intel_crtc); > > - intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); > > = > > for_each_encoder_on_crtc(dev, crtc, encoder) > > encoder->enable(encoder); > > @@ -4564,6 +4565,9 @@ static void i9xx_crtc_enable(struct drm_crtc *crt= c) > > = > > intel_crtc->active =3D true; > > = > > + if (!IS_GEN2(dev)) > > + intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); > > + > > for_each_encoder_on_crtc(dev, crtc, encoder) > > if (encoder->pre_enable) > > encoder->pre_enable(encoder); > > @@ -4576,13 +4580,22 @@ static void i9xx_crtc_enable(struct drm_crtc *c= rtc) > > = > > intel_update_watermarks(crtc); > > intel_enable_pipe(intel_crtc); > > - intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); > > = > > for_each_encoder_on_crtc(dev, crtc, encoder) > > encoder->enable(encoder); > > = > > intel_crtc_enable_planes(crtc); > > = > > + /* > > + * Gen2 reports pipe underruns whenever all planes are disabled. > > + * So don't enable underrun reporting before at least some planes > > + * are enabled. > > + * FIXME: Need to fix the logic to work when we turn off all planes > > + * but leave the pipe running. > > + */ > > + if (IS_GEN2(dev)) > > + intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); > = > I guess we should do this as part of the nuclear pageflip code iff all > planes are off. But for now this looks good enough imo. Forgotten to mention: All patches merged now, thanks. -Daniel -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch