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@xxxxxxxxx>
Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@xxxxxxxxx>
On 06/06, Daniel Vetter wrote:
> In
>
> commit def35e7c592616bc09be328de8795e5e624a3cf8
> Author: Shayenne Moura <shayenneluzmoura@xxxxxxxxx>
> 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@xxxxxxxxx>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@xxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
