[PATCH 00/12] irq vblank handling rework

[Thierry Reding]

On Wed, May 21, 2014 at 10:35:59AM +0200, Daniel Vetter wrote:
> On Wed, May 21, 2014 at 11:26:55AM +0300, Ville Syrj?l? wrote:
> > For everything but the reset_vblank_counter() thing:
> > Reviewed-by: Ville Syrj?l? 
> > 
> > And another caveat: I only glanced at the crtc_helper stuff. It looks
> > sane but I didn't go reading through the drivers to figure out if they
> > would fall over or work.
> 
> Oh, the rfc was really just that. I don't have any plans to burn my hands
> trying to merge those patches ;-) Especially since interest from non-i915
> hackers seems to be low.

I think that's mostly because nobody except i915 has a testsuite that
would even remotely be able to uncover any of these issues. =)

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

[PATCH 00/12] irq vblank handling rework

[Ville Syrjälä]

On Wed, May 21, 2014 at 10:35:59AM +0200, Daniel Vetter wrote:
> On Wed, May 21, 2014 at 11:26:55AM +0300, Ville Syrj?l? wrote:
> > For everything but the reset_vblank_counter() thing:
> > Reviewed-by: Ville Syrj?l? 
> > 
> > And another caveat: I only glanced at the crtc_helper stuff. It looks
> > sane but I didn't go reading through the drivers to figure out if they
> > would fall over or work.
> 
> Oh, the rfc was really just that. I don't have any plans to burn my hands
> trying to merge those patches ;-) Especially since interest from non-i915
> hackers seems to be low.
> 
> > About the reset_vblank_counter(), I think we might still need it to keep
> > the counter sane when the power well goes off. But I think we have
> > other problems on that front esp. with suspend to disk. There the counter
> > should definitely get reset on all platforms, so we migth apply any kind
> > of diff to the user visible value. The fix would likely be to skip the
> > diff adjustment when resuming.
> > 
> > I tried to take a quick look at that yesterday but there was something
> > really fishy happening as the code didn't seem to observe the wraparound
> > at all, even though I confirmed w/ intel_reg_read that it definitely
> > did appear to wrap. I'll take another look at it today.
> > 
> > Another idea might be to rip out the diff adjustment altogether. That
> > should just mean that the user visible counter wouldn't advance at all
> > between drm_vblank_off() and drm_vblank_on(). But that might actually
> > be the sane thing to do. Maybe we should just do a +1 there to make
> > sure we don't report the same value before and after modeset. It should
> > fix both the suspend problems and the power well problem.
> 
> Hm, like I've mentioned yesterday on irc the tests I have actually pass,
> at least if I throw your sanitize_crtc patch on top. vblank frame counter
> values monotonically increase across suspend/resume, runtime pm and
> anything else I manged to throw at it. And the limit in the test is 100
> frames later, but I've only observed a few tens at most.
> 
> So I think the code as-is actually works. Whether intentional or not is
> still under dispute though ;-)
> 
> The real problem I have with the hsw counter reset is that the same issue
> should affect _any_ platform where we support runtime pm. Like snb or byt.
> But the code isn't there. Also if we have such a bug then it will also
> affect hibernate and suspend to disk. Which means that this should be done
> in drm_crtc_vblank_off/on, not in the guts of some random platforms
> runtime pm code.
> 
> So in my opinion the hsw vblank_count reset code needs to go anyway
> because:
> - Either it isn't need any more (and we have the tests for this now) and
>   it's just cargo-culted duct-tape.
> - Or we need, but then it's in the wrong spot.
> 
> Given that can you reconsider that patch please?

Yeah. So as discussed on irc I think the right fix would be to sample
the current counter in drm_vblank_on(), stick it into
dev->vblank[crtc].last, but skip the diff adjustment to the user visible
counter (maybe just +1 to make sure we never report the same value on
both sides of a modeset). That should cover both the suspend case and the
power well case. I can go and experiment with this a bit...

So I agree that the current code isn't the way things should be done.
And since I now have an idea how it should be done, I'm fine with
ripping the current thing out. So you can add:
Reviewed-by: Ville Syrj?l? 

-- 
Ville Syrj?l?
Intel OTC

[PATCH 00/12] irq vblank handling rework

[Ville Syrjälä]

For everything but the reset_vblank_counter() thing:
Reviewed-by: Ville Syrj?l? 

And another caveat: I only glanced at the crtc_helper stuff. It looks
sane but I didn't go reading through the drivers to figure out if they
would fall over or work.

About the reset_vblank_counter(), I think we might still need it to keep
the counter sane when the power well goes off. But I think we have
other problems on that front esp. with suspend to disk. There the counter
should definitely get reset on all platforms, so we migth apply any kind
of diff to the user visible value. The fix would likely be to skip the
diff adjustment when resuming.

I tried to take a quick look at that yesterday but there was something
really fishy happening as the code didn't seem to observe the wraparound
at all, even though I confirmed w/ intel_reg_read that it definitely
did appear to wrap. I'll take another look at it today.

Another idea might be to rip out the diff adjustment altogether. That
should just mean that the user visible counter wouldn't advance at all
between drm_vblank_off() and drm_vblank_on(). But that might actually
be the sane thing to do. Maybe we should just do a +1 there to make
sure we don't report the same value before and after modeset. It should
fix both the suspend problems and the power well problem.

On Wed, May 14, 2014 at 08:51:02PM +0200, Daniel Vetter wrote:
> Hi all,
> 
> This is Ville's vblank rework series, slightly pimped. I've added kerneldoc,
> some polish and also some additional nasty igt testcases for these crtc on/off
> vs vblank issues. Seems all to hold together nicely.
> 
> Now one thing I wanted to do is roll this out across all drm drivers, but that
> looks ugly: Many drivers don't support vblanks really, and I couldn't fully
> audit whether they'd e.g. blow up when userspace tries to use the vblank wait
> ioctl. But I think we want to have more sanity since otherwise userspace is
> doomed to carry countless ugly hacks around forever.
> 
> The last two patches are my attempt at that. By doing the drm_vblank_on/off
> dance in the crtc helpers after we know that the crtc is on/off we should have
> at least a somewhat sane baseline behaviour. Note that since drm_vblank_on/off
> are idempotent drivers are free to call these functions in their callback at 
> an
> earlier time, where it makes more sense. But at least it's guaranteed to 
> happen.
> 
> Otoh drivers still need to manually complete pageflips, so this doesn't solve
> all the issues. But until we have a solid cross-driver testsuite for these
> corner-cases (all the interesting stuff happens when you race vblank waits
> against concurrent modesets, dpms, or system/runtime suspend/resume) we're
> pretty much guaranteed to have some that works at best occasionally and is
> guaranteed to have different behaviour in corner cases. Note that the patches
> won't degrade behaviour for existing drivers, the drm core changes simply 
> allow
> us to finally fix things up correctly.
> 
> I guess in a way this is a more generic discussion for the drm subsystem at
> large.
> 
> Coments and review highly welcome.
> 
> Cheers, Daniel
> 
> Daniel Vetter (8):
>   drm/i915: Remove drm_vblank_pre/post_modeset calls
>   drm/doc: Discourage usage of MODESET_CTL ioctl
>   drm/irq: kerneldoc polish
>   drm/irq: Add kms-native crtc interface functions
>   drm/i915: rip our vblank reset hacks for runtime PM
>   drm/i915: Accurately initialize fifo underrun state on gmch platforms
>   [RFC] drm/irq: More robustness in drm_vblank_on|off
>   [RFC] drm/crtc-helper: Enforce sane vblank counter semantics
> 
> Peter Hurley (1):
>   drm: Use correct spinlock flavor in drm_vblank_get()
> 
> Ville Syrj?l? (3):
>   drm: Make the vblank disable timer per-crtc
>   drm: Make blocking vblank wait return when the vblank interrupts get
> disabled
>   drm: Add drm_vblank_on()
> 
>  Documentation/DocBook/drm.tmpl   |  16 +-
>  drivers/gpu/drm/drm_crtc_helper.c|  12 ++
>  drivers/gpu/drm/drm_irq.c| 377 
> ++-
>  drivers/gpu/drm/i915/i915_irq.c  |   4 +-
>  drivers/gpu/drm/i915/intel_display.c |  36 ++--
>  drivers/gpu/drm/i915/intel_drv.h |   2 -
>  drivers/gpu/drm/i915/intel_pm.c  |  40 
>  include/drm/drmP.h   |  10 +-
>  8 files changed, 341 insertions(+), 156 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrj?l?
Intel OTC

[PATCH 00/12] irq vblank handling rework

[Daniel Vetter]

On Wed, May 21, 2014 at 11:26:55AM +0300, Ville Syrj?l? wrote:
> For everything but the reset_vblank_counter() thing:
> Reviewed-by: Ville Syrj?l? 
> 
> And another caveat: I only glanced at the crtc_helper stuff. It looks
> sane but I didn't go reading through the drivers to figure out if they
> would fall over or work.

Oh, the rfc was really just that. I don't have any plans to burn my hands
trying to merge those patches ;-) Especially since interest from non-i915
hackers seems to be low.

> About the reset_vblank_counter(), I think we might still need it to keep
> the counter sane when the power well goes off. But I think we have
> other problems on that front esp. with suspend to disk. There the counter
> should definitely get reset on all platforms, so we migth apply any kind
> of diff to the user visible value. The fix would likely be to skip the
> diff adjustment when resuming.
> 
> I tried to take a quick look at that yesterday but there was something
> really fishy happening as the code didn't seem to observe the wraparound
> at all, even though I confirmed w/ intel_reg_read that it definitely
> did appear to wrap. I'll take another look at it today.
> 
> Another idea might be to rip out the diff adjustment altogether. That
> should just mean that the user visible counter wouldn't advance at all
> between drm_vblank_off() and drm_vblank_on(). But that might actually
> be the sane thing to do. Maybe we should just do a +1 there to make
> sure we don't report the same value before and after modeset. It should
> fix both the suspend problems and the power well problem.

Hm, like I've mentioned yesterday on irc the tests I have actually pass,
at least if I throw your sanitize_crtc patch on top. vblank frame counter
values monotonically increase across suspend/resume, runtime pm and
anything else I manged to throw at it. And the limit in the test is 100
frames later, but I've only observed a few tens at most.

So I think the code as-is actually works. Whether intentional or not is
still under dispute though ;-)

The real problem I have with the hsw counter reset is that the same issue
should affect _any_ platform where we support runtime pm. Like snb or byt.
But the code isn't there. Also if we have such a bug then it will also
affect hibernate and suspend to disk. Which means that this should be done
in drm_crtc_vblank_off/on, not in the guts of some random platforms
runtime pm code.

So in my opinion the hsw vblank_count reset code needs to go anyway
because:
- Either it isn't need any more (and we have the tests for this now) and
  it's just cargo-culted duct-tape.
- Or we need, but then it's in the wrong spot.

Given that can you reconsider that patch please?

Thanks, Daniel

> 
> On Wed, May 14, 2014 at 08:51:02PM +0200, Daniel Vetter wrote:
> > Hi all,
> > 
> > This is Ville's vblank rework series, slightly pimped. I've added kerneldoc,
> > some polish and also some additional nasty igt testcases for these crtc 
> > on/off
> > vs vblank issues. Seems all to hold together nicely.
> > 
> > Now one thing I wanted to do is roll this out across all drm drivers, but 
> > that
> > looks ugly: Many drivers don't support vblanks really, and I couldn't fully
> > audit whether they'd e.g. blow up when userspace tries to use the vblank 
> > wait
> > ioctl. But I think we want to have more sanity since otherwise userspace is
> > doomed to carry countless ugly hacks around forever.
> > 
> > The last two patches are my attempt at that. By doing the drm_vblank_on/off
> > dance in the crtc helpers after we know that the crtc is on/off we should 
> > have
> > at least a somewhat sane baseline behaviour. Note that since 
> > drm_vblank_on/off
> > are idempotent drivers are free to call these functions in their callback 
> > at an
> > earlier time, where it makes more sense. But at least it's guaranteed to 
> > happen.
> > 
> > Otoh drivers still need to manually complete pageflips, so this doesn't 
> > solve
> > all the issues. But until we have a solid cross-driver testsuite for these
> > corner-cases (all the interesting stuff happens when you race vblank waits
> > against concurrent modesets, dpms, or system/runtime suspend/resume) we're
> > pretty much guaranteed to have some that works at best occasionally and is
> > guaranteed to have different behaviour in corner cases. Note that the 
> > patches
> > won't degrade behaviour for existing drivers, the drm core changes simply 
> > allow
> > us to finally fix things up correctly.
> > 
> > I guess in a way this is a more generic discussion for the drm subsystem at
> > large.
> > 
> > Coments and review highly welcome.
> > 
> > Cheers, Daniel
> > 
> > Daniel Vetter (8):
> >   drm/i915: Remove drm_vblank_pre/post_modeset calls
> >   drm/doc: Discourage usage of MODESET_CTL ioctl
> >   drm/irq: kerneldoc polish
> >   drm/irq: Add kms-native crtc interface functions
> >   drm/i915: rip our vblank reset hacks for runtime PM
> >   drm/i915: Accurately initialize fifo 

[PATCH 00/12] irq vblank handling rework

[Daniel Vetter]

Hi all,

This is Ville's vblank rework series, slightly pimped. I've added kerneldoc,
some polish and also some additional nasty igt testcases for these crtc on/off
vs vblank issues. Seems all to hold together nicely.

Now one thing I wanted to do is roll this out across all drm drivers, but that
looks ugly: Many drivers don't support vblanks really, and I couldn't fully
audit whether they'd e.g. blow up when userspace tries to use the vblank wait
ioctl. But I think we want to have more sanity since otherwise userspace is
doomed to carry countless ugly hacks around forever.

The last two patches are my attempt at that. By doing the drm_vblank_on/off
dance in the crtc helpers after we know that the crtc is on/off we should have
at least a somewhat sane baseline behaviour. Note that since drm_vblank_on/off
are idempotent drivers are free to call these functions in their callback at an
earlier time, where it makes more sense. But at least it's guaranteed to happen.

Otoh drivers still need to manually complete pageflips, so this doesn't solve
all the issues. But until we have a solid cross-driver testsuite for these
corner-cases (all the interesting stuff happens when you race vblank waits
against concurrent modesets, dpms, or system/runtime suspend/resume) we're
pretty much guaranteed to have some that works at best occasionally and is
guaranteed to have different behaviour in corner cases. Note that the patches
won't degrade behaviour for existing drivers, the drm core changes simply allow
us to finally fix things up correctly.

I guess in a way this is a more generic discussion for the drm subsystem at
large.

Coments and review highly welcome.

Cheers, Daniel

Daniel Vetter (8):
  drm/i915: Remove drm_vblank_pre/post_modeset calls
  drm/doc: Discourage usage of MODESET_CTL ioctl
  drm/irq: kerneldoc polish
  drm/irq: Add kms-native crtc interface functions
  drm/i915: rip our vblank reset hacks for runtime PM
  drm/i915: Accurately initialize fifo underrun state on gmch platforms
  [RFC] drm/irq: More robustness in drm_vblank_on|off
  [RFC] drm/crtc-helper: Enforce sane vblank counter semantics

Peter Hurley (1):
  drm: Use correct spinlock flavor in drm_vblank_get()

Ville Syrj?l? (3):
  drm: Make the vblank disable timer per-crtc
  drm: Make blocking vblank wait return when the vblank interrupts get
disabled
  drm: Add drm_vblank_on()

 Documentation/DocBook/drm.tmpl   |  16 +-
 drivers/gpu/drm/drm_crtc_helper.c|  12 ++
 drivers/gpu/drm/drm_irq.c| 377 ++-
 drivers/gpu/drm/i915/i915_irq.c  |   4 +-
 drivers/gpu/drm/i915/intel_display.c |  36 ++--
 drivers/gpu/drm/i915/intel_drv.h |   2 -
 drivers/gpu/drm/i915/intel_pm.c  |  40 
 include/drm/drmP.h   |  10 +-
 8 files changed, 341 insertions(+), 156 deletions(-)

-- 
1.8.3.1