Re: [PATCH 07/12] drm/irq: kerneldoc polish

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, May 15, 2014 at 01:44:04PM +0900, Michel Dänzer wrote:
> On 15.05.2014 03:51, Daniel Vetter wrote:
> > @@ -964,8 +1005,13 @@ EXPORT_SYMBOL(drm_vblank_off);
> >  
> >  /**
> >   * drm_vblank_on - enable vblank events on a CRTC
> > - * @dev: DRM device
> > + * @dev: drm device
> >   * @crtc: CRTC in question
> > + *
> > + * This functions restores the vblank interrupt state captured with
> > + * drm_vblank_off() again. Note that calls to drm_vblank_on() and
> > + * drm_vblank_off() can be unbalanced and so can also be unconditionaly called
> > + * in driver load code to reflect the current hardware state of the crtc.
> 
> Given this description, maybe something like drm_vblank_save/restore
> would describe better what these functions do than drm_vblank_off/on?

It also enables/disables the vblank machinery itself, which at least in
i915 will be important shortly - we slowly switch over our code to be more
interrupt driven, including the initial plane enabling/disabling in
modeset changes.

> > @@ -981,11 +1027,26 @@ EXPORT_SYMBOL(drm_vblank_on);
> >  
> >  /**
> >   * drm_vblank_pre_modeset - account for vblanks across mode sets
> > - * @dev: DRM device
> > + * @dev: drm device
> >   * @crtc: CRTC in question
> >   *
> >   * Account for vblank events across mode setting events, which will likely
> >   * reset the hardware frame counter.
> > + *
> > + * This is done by grabbing a temporary vblank reference to ensure that the
> > + * vblank interrupt keeps running across the modeset sequence. With this the
> > + * software-side vblank frame counting will ensure that there are no jumps or
> > + * discontinuities.
> > + *
> > + * Unfortunately this approach is racy and also doesn't work when the vblank
> > + * interrupt stops running, e.g. across system suspend resume. It is therefore
> > + * highly recommended that drivers use the newer drm_vblank_off() and
> > + * drm_vblank_on() instead. drm_vblank_pre_modeset() only works correctly when
> > + * using "cooked" software vblank frame counters and not relying on any hardware
> > + * counters.
> 
> That last statement is not true IME with radeon[0].
> 
> Basically, it sounds to me like drm_vblank_off/on do more or less what
> drm_vblank_pre/post_modeset are intended to do (e.g. the latter can also
> be nested arbitrarily). Still not really sure why we need two sets of
> these instead of fixing any problems in one set.

The problem is that the driver situation is a mess. So the right plan imo
is:
1) Create new functions that work, beat on them.
2) Convert all drivers.
3) Rip out the old stuff.

I've tried to look into this a bit and decided that this isn't something I
can do without the hardware at hand and a few solid tests. So this series
is 1) done for i915, with an rfc for 2).

Also there's the issue of old ums using the same stuff and I think we
shouldn't touch that can of worms at all.

> [0] Though we certainly don't have as rigorous testing for that as you
> guys do in intel-gpu-tools. Any chance some of that could be moved to
> somewhere more generic?

igt is mostly ready - what we need is some rather thin abstraction for the
kms tests to run also on other drivers, with dumb objects. Otherwise all
the test framework can cope with random bits not being there and skipping
those subtests, e.g. the CRC based stuff.

I don't want to extract the generic parts of the tests into a different
repo (at least not if there's not _lots_ of people working on them) since
the code sharing we currently do between tests is fairly massive.

But I'm willing to deal with the hassle of supporting other drivers for
e.g. the kms tests. And I think it would make a nice gsoc project. But
it's definitely not something I can throw intel resource (or too much of
my own time) at ;-)

Having a shared set of tests which clearly spells out all the corner cases
and tests for races would imo be awesome and greatly improve the overall
health of the drm/kms drivers and wider ecosystem.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch