[PATCH] drm/vkms: Forward timer right after drm_crtc_handle_vblank

[Rodrigo Siqueira]

Hi Daniel,

Thanks for the patch, and for the great explanation embedded to it.

I tested it here, and everything worked like a charm.

Is it ok if I push it to drm-misc?

Tested-by: Rodrigo Siqueira <rodrigosiqueiramelo at gmail.com>
Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo at gmail.com>

On 06/06, Daniel Vetter wrote:
> In
> 
> commit def35e7c592616bc09be328de8795e5e624a3cf8
> Author: Shayenne Moura <shayenneluzmoura at gmail.com>
> Date:   Wed Jan 30 14:06:36 2019 -0200
> 
>     drm/vkms: Bugfix extra vblank frame
> 
> we fixed the vblank counter to give accurate results outside of
> drm_crtc_handle_vblank, which fixed bugs around vblank timestamps
> being off-by-one and causing the vblank counter to jump when it
> shouldn't.
> 
> The trouble is that this completely broke crc generation. Shayenne and
> Rodrigo tracked this down to the vblank timestamp going backwards in
> time somehow. Which then resulted in an underflow in drm_vblank.c
> code, which resulted in all kinds of things breaking really badly.
> 
> The reason for this is that once we've called drm_crtc_handle_vblank
> and the hrtimer isn't forwarded yet, we're returning a vblank
> timestamp in the past. This race is really hard to hit since it's
> small, except when you enable crc generation: In that case there's a
> call to drm_crtc_accurate_vblank right in-betwen, so we're guaranteed
> to hit the bug.
> 
> The fix is to roll the hrtimer forward _before_ we do the vblank
> processing (which has a side-effect of incrementing the vblank
> counter), and we always subtract one frame from the hrtimer - since
> now it's always one frame in the future.
> 
> To make sure we don't hit this again also add a WARN_ON checking for
> whether our timestamp is somehow moving into the past, which is never
> should.
> 
> This also aligns more with how real hw works:
> 1. first all registers are updated with the new timestamp/vblank
> counter values.
> 2. then an interrupt is generated
> 3. kernel interrupt handler eventually fires.
> 
> So doing this aligns vkms closer with what drm_vblank.c expects.
> Document this also in a comment.
> 
> Cc: Shayenne Moura <shayenneluzmoura at gmail.com>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo at gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  drivers/gpu/drm/vkms/vkms_crtc.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 7508815fac11..1bbe099b7db8 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -15,6 +15,10 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
>  
>  	spin_lock(&output->lock);
>  
> +	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> +					  output->period_ns);
> +	WARN_ON(ret_overrun != 1);
> +
>  	ret = drm_crtc_handle_vblank(crtc);
>  	if (!ret)
>  		DRM_ERROR("vkms failure on handling vblank");
> @@ -35,10 +39,6 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
>  			DRM_WARN("failed to queue vkms_crc_work_handle");
>  	}
>  
> -	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> -					  output->period_ns);
> -	WARN_ON(ret_overrun != 1);
> -
>  	spin_unlock(&output->lock);
>  
>  	return HRTIMER_RESTART;
> @@ -74,11 +74,21 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
>  {
>  	struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
>  	struct vkms_output *output = &vkmsdev->output;
> +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  
>  	*vblank_time = output->vblank_hrtimer.node.expires;
>  
> -	if (!in_vblank_irq)
> -		*vblank_time -= output->period_ns;
> +	if (WARN_ON(*vblank_time == vblank->time))
> +		return true;
> +
> +	/*
> +	 * To prevent races we roll the hrtimer forward before we do any
> +	 * interrupt processing - this is how real hw works (the interrupt is
> +	 * only generated after all the vblank registers are updated) and what
> +	 * the vblank core expects. Therefore we need to always correct the
> +	 * timestampe by one frame.
> +	 */
> +	*vblank_time -= output->period_ns;
>  
>  	return true;
>  }
> -- 
> 2.20.1
> 

-- 
Rodrigo Siqueira
https://siqueira.tech
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20190606/29b7d6de/attachment.sig>