Re: [PATCH 3/3] drm: Peek at the current counter/timestamp for vblank queries

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Michel Dänzer" <michel@daenzer.net>,
	dri-devel@lists.freedesktop.org,
	"Laurent Pinchart" <laurent.pinchart@ideasonboard.com>,
	"Dave Airlie" <airlied@redhat.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/3] drm: Peek at the current counter/timestamp for vblank queries
Date: Wed, 15 Mar 2017 23:06:32 +0200	[thread overview]
Message-ID: <20170315210632.GZ31595@intel.com> (raw)
In-Reply-To: <20170315204027.20160-3-chris@chris-wilson.co.uk>

On Wed, Mar 15, 2017 at 08:40:27PM +0000, 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.
> 
> This is dependent upon the both the hardware and driver to provide the
> proper barriers to facilitate reading our bookkeeping outside of the
> vblank interrupt and outside of the explicit vblank locks.
> 
> 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 53a526c7b24d..c55abce152a2 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1559,6 +1559,17 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  	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.
>   *
> @@ -1608,6 +1619,21 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
>  
>  	vblank = &dev->vblank[pipe];
>  
> +	/* If the counter is currently enabled and accurate, short-circuit queries
> +	 * to return the cached timestamp of the last vblank.
> +	 */
> +	if (dev->vblank_disable_immediate &&
> +	    drm_wait_vblank_is_query(vblwait) &&
> +	    vblank->enabled) {

Hmm. I'm wondering if this could give us something stale if we're just
about to enable the vblank interrupt.

We seem to set enabled=true before we update the count/ts. So I'm 
thinking we'd need to flip those around and make sure proper barriers
are in place.

> +		struct timeval now;
> +
> +		vblwait->reply.sequence =
> +			drm_vblank_count_and_time(dev, pipe, &now);
> +		vblwait->reply.tval_sec = now.tv_sec;
> +		vblwait->reply.tval_usec = now.tv_usec;
> +		return 0;
> +	}
> +
>  	ret = drm_vblank_get(dev, pipe);
>  	if (ret) {
>  		DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
> -- 
> 2.11.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx