Re: [PATCH 2/6] drm/vkms: Use vblank timer

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 05/06/2025 à 17:24, Thomas Zimmermann a écrit :
> Replace vkms' vblank timer with the DRM implementation. The DRM
> code is mostly identical.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
Tested-by: Louis Chauvet <louis.chauvet@bootlin>

> ---
>   drivers/gpu/drm/vkms/vkms_crtc.c | 49 +++++++-------------------------
>   drivers/gpu/drm/vkms/vkms_drv.h  |  6 ++--
>   2 files changed, 15 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 8c9898b9055d..5b7829e8c900 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -10,22 +10,14 @@
>   
>   #include "vkms_drv.h"
>   
> -static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> +static bool vkms_crtc_handle_vblank(struct drm_crtc *crtc)
>   {
> -	struct vkms_output *output = container_of(timer, struct vkms_output,
> -						  vblank_hrtimer);
> -	struct drm_crtc *crtc = &output->crtc;
> +	struct vkms_output *output = drm_crtc_to_vkms_output(crtc);
>   	struct vkms_crtc_state *state;
> -	u64 ret_overrun;
>   	bool ret, fence_cookie;
>   
>   	fence_cookie = dma_fence_begin_signalling();
>   
> -	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> -					  output->period_ns);
> -	if (ret_overrun != 1)
> -		pr_warn("%s: vblank timer overrun\n", __func__);
> -
>   	spin_lock(&output->lock);
>   	ret = drm_crtc_handle_vblank(crtc);
>   	if (!ret)
> @@ -57,18 +49,14 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
>   
>   	dma_fence_end_signalling(fence_cookie);
>   
> -	return HRTIMER_RESTART;
> +	return true;
>   }
>   
>   static int vkms_enable_vblank(struct drm_crtc *crtc)
>   {
> -	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
>   	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
>   
> -	hrtimer_setup(&out->vblank_hrtimer, &vkms_vblank_simulate, CLOCK_MONOTONIC,
> -		      HRTIMER_MODE_REL);
> -	out->period_ns = ktime_set(0, vblank->framedur_ns);
> -	hrtimer_start(&out->vblank_hrtimer, out->period_ns, HRTIMER_MODE_REL);
> +	drm_vblank_timer_start(&out->vtimer);
>   
>   	return 0;
>   }
> @@ -77,7 +65,7 @@ static void vkms_disable_vblank(struct drm_crtc *crtc)
>   {
>   	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
>   
> -	hrtimer_cancel(&out->vblank_hrtimer);
> +	drm_vblank_timer_cancel(&out->vtimer);
>   }
>   
>   static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
> @@ -85,28 +73,9 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
>   				      bool in_vblank_irq)
>   {
>   	struct vkms_output *output = drm_crtc_to_vkms_output(crtc);
> -	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
> -
> -	if (!READ_ONCE(vblank->enabled)) {
> -		*vblank_time = ktime_get();
> -		return true;
> -	}
> -
> -	*vblank_time = READ_ONCE(output->vblank_hrtimer.node.expires);
>   
> -	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;
> +	return drm_vblank_timer_get_vblank_timestamp(&output->vtimer, max_error,
> +						     vblank_time, in_vblank_irq);
>   }
>   
>   static struct drm_crtc_state *
> @@ -274,6 +243,7 @@ struct vkms_output *vkms_crtc_init(struct drm_device *dev, struct drm_plane *pri
>   {
>   	struct vkms_output *vkms_out;
>   	struct drm_crtc *crtc;
> +	struct drm_vblank_timer *vtimer;
>   	int ret;
>   
>   	vkms_out = drmm_crtc_alloc_with_planes(dev, struct vkms_output, crtc,
> @@ -285,6 +255,7 @@ struct vkms_output *vkms_crtc_init(struct drm_device *dev, struct drm_plane *pri
>   	}
>   
>   	crtc = &vkms_out->crtc;
> +	vtimer = &vkms_out->vtimer;
>   
>   	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
>   
> @@ -305,5 +276,7 @@ struct vkms_output *vkms_crtc_init(struct drm_device *dev, struct drm_plane *pri
>   	if (!vkms_out->composer_workq)
>   		return ERR_PTR(-ENOMEM);
>   
> +	drmm_vblank_timer_init(vtimer, crtc, vkms_crtc_handle_vblank);
> +
>   	return vkms_out;
>   }
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index a74a7fc3a056..126016898285 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -10,6 +10,7 @@
>   #include <drm/drm_gem.h>
>   #include <drm/drm_gem_atomic_helper.h>
>   #include <drm/drm_encoder.h>
> +#include <drm/drm_vblank_timer.h>
>   #include <drm/drm_writeback.h>
>   
>   #define DEFAULT_DEVICE_NAME "vkms"
> @@ -180,8 +181,9 @@ struct vkms_output {
>   	struct drm_crtc crtc;
>   	struct drm_writeback_connector wb_connector;
>   	struct drm_encoder wb_encoder;
> -	struct hrtimer vblank_hrtimer;
> -	ktime_t period_ns;
> +	struct drm_vblank_timer vtimer;
> +	struct drm_pending_vblank_event *event;
> +	/* ordered wq for composer_work */
>   	struct workqueue_struct *composer_workq;
>   	spinlock_t lock;
>   

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com