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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
