Re: [PATCH 2/2] drm: Shortcircuit vblank queries

[Mario Kleiner <mario.kleiner.de@gmail.com>]

On 04/05/2015 05:40 PM, Chris Wilson wrote:
> Bypass all the spinlocks and return the last timestamp and counter from
> the last vblank if the driver delcares that it is accurate (and stable
> across on/off), and the vblank is currently enabled.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Dave Airlie <airlied@redhat.com>,
> Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
> ---
>   drivers/gpu/drm/drm_irq.c | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index ba80b51b4b00..be9c210bb22e 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1538,6 +1538,17 @@ err_put:
>   	return ret;
>   }
>
> +static bool drm_wait_vblank_is_query(union drm_wait_vblank *vblwait)
> +{
> +	if (vblwait->request.sequence)
> +		return false;
> +
> +	return _DRM_VBLANK_RELATIVE ==
> +		(vblwait->request.type & (_DRM_VBLANK_TYPES_MASK |
> +					  _DRM_VBLANK_EVENT |
> +					  _DRM_VBLANK_NEXTONMISS));
> +}
> +
>   /*
>    * Wait for VBLANK.
>    *
> @@ -1587,6 +1598,21 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
>
>   	vblank = &dev->vblank[crtc];
>
> +	/* If the counter is currently enabled and accurate, short-circuit queries
> +	 * to return the cached timestamp of the last vblank.
> +	 */

Maybe somehow stress in the comment that this location in 
drm_wait_vblank is really the only place where it is ok'ish to call
drm_vblank_count_and_time() without wrapping it into a 
drm_vblank_get/put(), so nobody thinks this approach is ok anywhere else.

> +	if (dev->vblank_disable_immediate &&
> +	    drm_wait_vblank_is_query(vblwait) &&
> +	    vblank->enabled) {

You should also check for (drm_vblank_offdelay != 0) whenever checking 
for dev->vblank_disable_immediate. This is so one can override all the
vblank_disable_immediate related logic via the drm vblankoffdelay module 
parameter, both for debugging and as a safety switch for desparate users 
in case some driver+gpu combo screws up wrt. immediate disable and that 
makes it into distro kernels.

The other thing i'm not sure is if it wouldn't be a good idea to have 
some kind of write memory barrier in vblank_disable_and_save() after 
setting vblank->enabled = false; and some read memory barrier here 
before your check for vblank->enabled? I don't have a feeling for how 
much time can pass between one core executing the disable and the other 
core receiving the news that vblank->enabled is no longer true if those 
bits run on different cores?

I've run your patches through my standard tests on x86_64 and they don't 
seem to introduce errors or more skipped frames. Normally it would be so 
wrong to do this without drm_vblank_get/put(), but i think here 
potential errors introduced wouldn't be worse than what a userspace 
client would see due to preemption or other execution delays at the 
wrong moment, so it's probably ok. But i don't know if lack of memory 
barriers etc. could introduce large delays and trouble on other 
architectures?

> +		struct timeval now;
> +
> +		vblwait->reply.sequence =
> +			drm_vblank_count_and_time(dev, crtc, &now);
> +		vblwait->reply.tval_sec = now.tv_sec;
> +		vblwait->reply.tval_usec = now.tv_usec;

Have some DRM_DEBUG here, so one can follow the client doing the instant 
query through this path.

> +		return 0;
> +	}
> +
>   	ret = drm_vblank_get(dev, crtc);
>   	if (ret) {
>   		DRM_DEBUG("failed to acquire vblank counter, %d\n", ret);
>

With the above addressed i'd give you a Reviewed-and-tested-by, but it 
would be good if somebody else could look over it as well.

-mario
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel