[Intel-gfx] [PATCH 2/4] drm/i915: Drop pipe_enable checks in vblank funcs

[Daniel Vetter]

On Fri, Feb 13, 2015 at 12:37:27AM +0200, Imre Deak wrote:
> On Tue, 2015-02-03 at 11:30 +0100, Daniel Vetter wrote:
> > With Ville's rework to use drm_crtc_vblank_on/off the core will take
> > care of rejecting drm_vblank_get calls when the pipe is off. Also the
> > core won't call the get_vblank_counter hooks in that case either. And
> > since we've dropped ums support recently we can now remove these
> > hacks, yay!
> > 
> > Noticed while trying to answer questions Laurent had about how the new
> > atomic helpers work.
> > 
> > Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 52 -----------------------------------------
> >  1 file changed, 52 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 23bfe2232b6a..80f35dcffea4 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -492,31 +492,6 @@ static void i915_enable_asle_pipestat(struct drm_device *dev)
> >  	spin_unlock_irq(&dev_priv->irq_lock);
> >  }
> >  
> > -/**
> > - * i915_pipe_enabled - check if a pipe is enabled
> > - * @dev: DRM device
> > - * @pipe: pipe to check
> > - *
> > - * Reading certain registers when the pipe is disabled can hang the chip.
> > - * Use this routine to make sure the PLL is running and the pipe is active
> > - * before reading such registers if unsure.
> > - */
> > -static int
> > -i915_pipe_enabled(struct drm_device *dev, int pipe)
> > -{
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -
> > -	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > -		/* Locking is horribly broken here, but whatever. */
> > -		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> > -		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > -
> > -		return intel_crtc->active;
> > -	} else {
> > -		return I915_READ(PIPECONF(pipe)) & PIPECONF_ENABLE;
> > -	}
> > -}
> > -
> >  /*
> >   * This timing diagram depicts the video signal in and
> >   * around the vertical blanking period.
> > @@ -583,12 +558,6 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
> >  	unsigned long low_frame;
> >  	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
> >  
> > -	if (!i915_pipe_enabled(dev, pipe)) {
> > -		DRM_DEBUG_DRIVER("trying to get vblank count for disabled "
> > -				"pipe %c\n", pipe_name(pipe));
> > -		return 0;
> > -	}
> 
> Wouldn't it make sense to still keep these as an assert (by throwing
> away the !MODESET part in i915_pipe_enabled). To me it seems we could
> still get here with the pipe disabled at least via drm_irq_uninstall().

The problem I see with the i915_pipe_enable check is that we do noodle
around in modeset state structures from irq context. Which is not awesome
at all, so I'd like to get rid of it completely.

The code in drm_irq_uninstall is indeed buggy though, it should grab the
timestamp from the software side like drm_vblank_off does. Note that on
i915 this code can't run since we disable the pipe first, which will
already flush out any pending events. Of which there shouldn't be any left
when the driver unloads anyway.
-Daniel

> 
> > -
> >  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> >  		struct intel_crtc *intel_crtc =
> >  			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> > @@ -648,12 +617,6 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int reg = PIPE_FRMCOUNT_GM45(pipe);
> >  
> > -	if (!i915_pipe_enabled(dev, pipe)) {
> > -		DRM_DEBUG_DRIVER("trying to get vblank count for disabled "
> > -				 "pipe %c\n", pipe_name(pipe));
> > -		return 0;
> > -	}
> > -
> >  	return I915_READ(reg);
> >  }
> >  
> > @@ -2660,9 +2623,6 @@ static int i915_enable_vblank(struct drm_device *dev, int pipe)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	unsigned long irqflags;
> >  
> > -	if (!i915_pipe_enabled(dev, pipe))
> > -		return -EINVAL;
> > -
> >  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> >  	if (INTEL_INFO(dev)->gen >= 4)
> >  		i915_enable_pipestat(dev_priv, pipe,
> > @@ -2682,9 +2642,6 @@ static int ironlake_enable_vblank(struct drm_device *dev, int pipe)
> >  	uint32_t bit = (INTEL_INFO(dev)->gen >= 7) ? DE_PIPE_VBLANK_IVB(pipe) :
> >  						     DE_PIPE_VBLANK(pipe);
> >  
> > -	if (!i915_pipe_enabled(dev, pipe))
> > -		return -EINVAL;
> > -
> >  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> >  	ironlake_enable_display_irq(dev_priv, bit);
> >  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> > @@ -2697,9 +2654,6 @@ static int valleyview_enable_vblank(struct drm_device *dev, int pipe)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	unsigned long irqflags;
> >  
> > -	if (!i915_pipe_enabled(dev, pipe))
> > -		return -EINVAL;
> > -
> >  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> >  	i915_enable_pipestat(dev_priv, pipe,
> >  			     PIPE_START_VBLANK_INTERRUPT_STATUS);
> > @@ -2713,9 +2667,6 @@ static int gen8_enable_vblank(struct drm_device *dev, int pipe)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	unsigned long irqflags;
> >  
> > -	if (!i915_pipe_enabled(dev, pipe))
> > -		return -EINVAL;
> > -
> >  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> >  	dev_priv->de_irq_mask[pipe] &= ~GEN8_PIPE_VBLANK;
> >  	I915_WRITE(GEN8_DE_PIPE_IMR(pipe), dev_priv->de_irq_mask[pipe]);
> > @@ -2767,9 +2718,6 @@ static void gen8_disable_vblank(struct drm_device *dev, int pipe)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	unsigned long irqflags;
> >  
> > -	if (!i915_pipe_enabled(dev, pipe))
> > -		return;
> > -
> >  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> >  	dev_priv->de_irq_mask[pipe] |= GEN8_PIPE_VBLANK;
> >  	I915_WRITE(GEN8_DE_PIPE_IMR(pipe), dev_priv->de_irq_mask[pipe]);
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch