Re: [PATCH 2/2] drm/i915: Fix scanline counter fixup on BDW

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Mar 11, 2014 at 12:38:10PM +0100, Daniel Vetter wrote:
> On Tue, Mar 11, 2014 at 12:58:46PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The display interrupts changed on BDW, so the current ILK-HSW specific
> > code in ilk_pipe_in_vblank_locked() doesn't work there. Add the required
> > bits for BDW, and while at it, change the existing code to use nicer
> > looking vblank status bit macros.
> > 
> > Also remove the now stale __raw_i915_read16() definition which was
> > left over from the failed gen2 ISR experiment.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=73962
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 30 +++++++++++-------------------
> >  1 file changed, 11 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index d4c952d..ec9b8a4 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -618,33 +618,25 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe)
> >  
> >  /* raw reads, only for fast reads of display block, no need for forcewake etc. */
> 
> fast reads in context with mmio transactions sounds like a fairly crazy
> oxymoron. And our I915_READ functions should already dtrt wrt not doing
> the forcewake dance for display registers, otherwise I'll consider it a
> bug.
> 
> Care to throw a patch on top to remove these guys completely?

We could have other junk in there like unclaimed register access
checks which could slow this down a bit. Although it seems we
don't do unclaimed register checks for reads currently.

I'm going to kill the ISR read anyway in the atomic sprite series,
so that'll get us back to just one mmio read per call, and so even
if we did a few extra mmio accesses for unclaimed register access
checks, it doesn't feel too expensive to me.

So yes, I could throw on an extra patch to do what you ask, but I'd
rather do that after the atomic sprite series is applied. These
patches are meant for -fixes anyway, so I want to keep them fairly
minimal.

> Thanks, Daniel
> 
> >  #define __raw_i915_read32(dev_priv__, reg__) readl((dev_priv__)->regs + (reg__))
> > -#define __raw_i915_read16(dev_priv__, reg__) readw((dev_priv__)->regs + (reg__))
> >  
> >  static bool ilk_pipe_in_vblank_locked(struct drm_device *dev, enum pipe pipe)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	uint32_t status;
> > -
> > -	if (INTEL_INFO(dev)->gen < 7) {
> > -		status = pipe == PIPE_A ?
> > -			DE_PIPEA_VBLANK :
> > -			DE_PIPEB_VBLANK;
> > +	int reg;
> > +
> > +	if (INTEL_INFO(dev)->gen >= 8) {
> > +		status = GEN8_PIPE_VBLANK;
> > +		reg = GEN8_DE_PIPE_ISR(pipe);
> > +	} else if (INTEL_INFO(dev)->gen >= 7) {
> > +		status = DE_PIPE_VBLANK_IVB(pipe);
> > +		reg = DEISR;
> >  	} else {
> > -		switch (pipe) {
> > -		default:
> > -		case PIPE_A:
> > -			status = DE_PIPEA_VBLANK_IVB;
> > -			break;
> > -		case PIPE_B:
> > -			status = DE_PIPEB_VBLANK_IVB;
> > -			break;
> > -		case PIPE_C:
> > -			status = DE_PIPEC_VBLANK_IVB;
> > -			break;
> > -		}
> > +		status = DE_PIPE_VBLANK(pipe);
> > +		reg = DEISR;
> >  	}
> >  
> > -	return __raw_i915_read32(dev_priv, DEISR) & status;
> > +	return __raw_i915_read32(dev_priv, reg) & status;
> >  }
> >  
> >  static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
> > -- 
> > 1.8.3.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC