Re: [PATCH 7/8] drm/irq: Implement a generic vblank_wait function

[Daniel Vetter <daniel@ffwll.ch>]

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