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

[Daniel Vetter]

On Mon, Jan 26, 2015 at 11:03:14PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Monday 26 January 2015 07:41:59 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.
> 
> After debugging a related issue in the omapdrm driver I think this is only 
> partially true. drm_vblank_off() will increase the vblank refcount, resulting 
> in drm_vblank_get() returning an error instead of calling the driver's 
> .enable_vblank operation. However, this only works if drm_vblank_off() has 
> been called, not when the pipe is off because it hasn't been turned on yet.

We recover this state at driver load by manually calling off/on. See intel_sanitize_crtc

> Consider this case. The driver has just been loaded, all pipes are off. An 
> application opens the DRM device node and closes it without enabling the pipe. 
> The lastclose operation will be called, and it will call 
> drm_fb_helper_restore_fbdev_mode_unlocked() to restore the CRTC. This triggers 
> the timeout warning in drm_wait_one_vblank with omapdrm when using the atomic 
> update transitional helpers. The callstack is as follows.
> 
> [<bf3b46f8>] (drm_wait_one_vblank [drm])
> [<bf429e50>] (drm_plane_helper_commit [drm_kms_helper])
> [<bf427ab0>] (drm_crtc_helper_set_mode [drm_kms_helper])
> [<bf428570>] (drm_crtc_helper_set_config [drm_kms_helper])
> [<bf3baa34>] (drm_mode_set_config_internal [drm])
> [<bf431374>] (restore_fbdev_mode [drm_kms_helper])
> [<bf432978>] (drm_fb_helper_restore_fbdev_mode_unlocked [drm_kms_helper])
> [<bf45d4f4>] (dev_lastclose [omapdrm])
> [<bf3b1218>] (drm_lastclose [drm])
> [<bf3b15e0>] (drm_release [drm])
> [<c0149104>] (__fput)
> [<c00571d4>] (task_work_run)
> [<c00114f8>] (do_work_pending)
> 
> The drm_crtc_vblank_get() call in drm_plane_helper_commit() will not return an 
> error as drm_vblank_off() has never been called so far.
> 
> The exact same issue obviously doesn't affect the i915 driver as it doesn't 
> use the transitional helpers, but a different issue could be present with the 
> same root cause.

We use the transitional helpers, code will land in drm-next soonish.

> Where do you think this should be fixed ?

Proper hw state readout for smooth booting is hard. I've tried to aim a
bit in that direction with the ->reset callbacks for atomic, but maybe we
should do a new helper inspired by i915 just for this kind of stuff ...

My idea would be to add ->read_hw_state hooks which will out the various
drm_*_state structs and then use that for both ->reset and for
cross-checking after modesets. And then we could shovel a bunch of the
common code to sanitize hw state into the shared ->reset functions.
But given that no one else but i915 tries really hard to reconstruct hw
state probably not yet worth it.
-Daniel

> 
> > 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 2399eaed2ca3..4761f7fe6fb1 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;
> > -	}
> > -
> >  	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]);
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

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