[Intel-gfx] [PATCH 5/5] drm/i915: Shuffle fifo underrun disable/enable points for gmch platforms

[Daniel Vetter]

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 at linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > 
> > 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 platforms.
> > 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älä <ville.syrjala at linux.intel.com>
> > ---
> >  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/i915/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_crtc *crtc)
> >  
> >  	intel_crtc->active = 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_crtc *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 *crtc)
> >  
> >  	intel_crtc->active = 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 *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);
> >  
> >  	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