Re: [PATCH 10/11] drm: Use vblank timestamps to guesstimate how many vblanks were missed

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 1573 bytes --]

On Mon, Sep 14, 2015 at 10:43:51PM +0300, ville.syrjala@linux.intel.com wrote:
[...]
> @@ -167,7 +238,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  	if (!rc)
>  		t_vblank = (struct timeval) {0, 0};
>  
> -	store_vblank(dev, pipe, diff, &t_vblank);
> +	store_vblank(dev, pipe, diff, &t_vblank, cur_vblank);

I think clearing the t_vblank is now wrong here. This breaks weston on
Tegra (and I think all other drivers that don't implement hardware
VBLANK timestamps). The reason is that if ->get_vblank_timestamp() isn't
implemented, rc will always be false at this point, hence resulting in a
zero VBLANK timestamp being reported to userspace.

In fact the comment, reproduced here because it's not in the patch
context:

	/*
	 * Only reinitialize corresponding vblank timestamp if high-precision query
	 * available and didn't fail. Otherwise reinitialize delayed at next vblank
	 * interrupt and assign 0 for now, to mark the vblanktimestamp as invalid.
	 */

is no longer accurate because this function is now called at the VBLANK
interrupt. If you want to keep invalidating timestamps for drivers that
implement VBLANK timestamp queries I think the condition needs to be
changed to something like:

	if (dev->get_vblank_timestamp && !rc)

and the comment updated to reflect the truth. Alternatively this code
can be removed, though I guess having the monotonic timestamp fallback
would be kind of missing the point.

I've tested either change and they both restore weston on Tegra.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx