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@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
