Re: [PATCH 2/4] drm/i915: Drop pipe_enable checks in vblank funcs

[Imre Deak <imre.deak@intel.com>]

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@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@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().

> -
>  	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]);


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel