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

["Michel Dänzer" <michel@daenzer.net>]

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.


> 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?


> +	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().


Looks good to me other than that.


-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel