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

Imre Deak
2015-02-12T22:37:27Z

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