On Wed, Jul 30, 2014 at 11:59:33AM +0900, Michel D?nzer wrote: > On 30.07.2014 06:32, Daniel Vetter wrote: > > As usual in both a crtc index and a struct drm_crtc * version. > > > > The function assumes that no one drivers their display below 10Hz, and > > it will complain if the vblank wait takes longer than that. > > > > v2: Also check dev->max_vblank_counter since some drivers register a > > fake get_vblank_counter function. > > What does that refer to? Can't find any other reference to > max_vblank_counter in the patch.
Oops, that was from an intermediate version. The v3: text somehow got lost where I've switched the code from directly calling the ->get_counter callback to using drm_vblank_counter, which will dtrt even when there's not hw counter available. Will augment the commit message. > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > > index 0de123afdb34..76024fdde452 100644 > > --- a/drivers/gpu/drm/drm_irq.c > > +++ b/drivers/gpu/drm/drm_irq.c > > @@ -999,6 +999,51 @@ void drm_crtc_vblank_put(struct drm_crtc *crtc) > > EXPORT_SYMBOL(drm_crtc_vblank_put); > > > > /** > > + * drm_vblank_wait - wait for one vblank > > + * @dev: DRM device > > + * @crtc: crtc index > > + * > > + * This waits for one vblank to pass on @crtc, using the irq driver > > interfaces. > > + * It is a failure to call this when the vblank irq for @crtc is disable, > > e.g. > > Spelling: 'disabled' > > > > + * due to lack of driver support or because the crtc is off. > > + */ > > +void drm_vblank_wait(struct drm_device *dev, int crtc) > > +{ > > + int ret; > > + u32 last; > > + > > + ret = drm_vblank_get(dev, crtc); > > + if (WARN_ON(ret)) > > + return; > > + > > + last = drm_vblank_count(dev, crtc); > > + > > +#define C (last != drm_vblank_count(dev, crtc)) > > + ret = wait_event_timeout(dev->vblank[crtc].queue, > > + C, msecs_to_jiffies(100)); > > + > > + WARN_ON(ret == 0); > > +#undef C > > What's the point of the C macro? Usually the conditions tend to overflow the 80 char limit, so I've adopted that pattern of extracting it into a local #define. I think it'll fit here, so will roll in. > > > > + drm_vblank_put(dev, crtc); > > +} > > +EXPORT_SYMBOL(drm_vblank_wait); > > + > > +/** > > + * drm_crtc_vblank_wait - wait for one vblank > > + * @crtc: DRM crtc > > + * > > + * This waits for one vblank to pass on @crtc, using the irq driver > > interfaces. > > + * It is a failure to call this when the vblank irq for @crtc is disable, > > e.g. > > Same typo as above. > > > > + * due to lack of driver support or because the crtc is off. > > + */ > > +void drm_crtc_vblank_wait(struct drm_crtc *crtc) > > +{ > > + drm_vblank_wait(crtc->dev, drm_crtc_index(crtc)); > > +} > > +EXPORT_SYMBOL(drm_crtc_vblank_wait); > > + > > +/** > > Maybe the function names should be *_vblank_wait_next() or something to > clarify the purpose and reduce potential confusion versus drm_wait_vblank(). Yeah that name is just transferred from the i915 driver. What about drm_wait_one_vblank()/drm_crtc_wait_one_vblank()? At least to my ear vblank_wait_next sounds backwards. > Looks good to me other than that. If you're ok with the name suggestion I'll polish & resend, thanks for the comments. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch