public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/amdgpu: Replace VKMS vblank timer with common implementation
@ 2026-04-20 12:51 Thomas Zimmermann
  2026-04-20 14:28 ` Alex Deucher
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Thomas Zimmermann @ 2026-04-20 12:51 UTC (permalink / raw)
  To: alexander.deucher, christian.koenig, airlied, simona
  Cc: amd-gfx, dri-devel, Thomas Zimmermann

Replace amdgpu's custom vblank timers with the shared implementation
in DRM's vblank code. Both are built upon hrtimers. The vblank logic
is identical.

The shared helpers contain all initialization internally. They also
handle a number of deadlocks and race conditions that are present in
amdgpu.

Also remove the set-but-unused field vsync_timer_enabled from struct
amdgpu_crtc.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h |   4 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c | 123 +----------------------
 2 files changed, 3 insertions(+), 124 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index 51ab1a332615..8069fc41cc7f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -38,7 +38,6 @@
 #include <drm/drm_probe_helper.h>
 #include <linux/i2c.h>
 #include <linux/i2c-algo-bit.h>
-#include <linux/hrtimer.h>
 #include "amdgpu_irq.h"
 
 #include <drm/display/drm_dp_mst_helper.h>
@@ -505,9 +504,6 @@ struct amdgpu_crtc {
 	u32 line_time;
 	u32 lb_vblank_lead_lines;
 	struct drm_display_mode hw_mode;
-	/* for virtual dce */
-	struct hrtimer vblank_timer;
-	enum amdgpu_interrupt_state vsync_timer_enabled;
 
 	int otg_inst;
 	struct drm_pending_vblank_event *event;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
index e54295b56282..9f497e9c65b8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
@@ -5,6 +5,7 @@
 #include <drm/drm_simple_kms_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_vblank.h>
+#include <drm/drm_vblank_helper.h>
 
 #include "amdgpu.h"
 #ifdef CONFIG_DRM_AMDGPU_SI
@@ -42,81 +43,6 @@ static const u32 amdgpu_vkms_formats[] = {
 	DRM_FORMAT_XRGB8888,
 };
 
-static enum hrtimer_restart amdgpu_vkms_vblank_simulate(struct hrtimer *timer)
-{
-	struct amdgpu_crtc *amdgpu_crtc = container_of(timer, struct amdgpu_crtc, vblank_timer);
-	struct drm_crtc *crtc = &amdgpu_crtc->base;
-	struct amdgpu_vkms_output *output = drm_crtc_to_amdgpu_vkms_output(crtc);
-	u64 ret_overrun;
-	bool ret;
-
-	ret_overrun = hrtimer_forward_now(&amdgpu_crtc->vblank_timer,
-					  output->period_ns);
-	if (ret_overrun != 1)
-		drm_warn(amdgpu_crtc->base.dev,
-			 "%s: vblank timer overrun count: %llu\n",
-			 __func__, ret_overrun);
-
-	ret = drm_crtc_handle_vblank(crtc);
-	/* Don't queue timer again when vblank is disabled. */
-	if (!ret)
-		return HRTIMER_NORESTART;
-
-	return HRTIMER_RESTART;
-}
-
-static int amdgpu_vkms_enable_vblank(struct drm_crtc *crtc)
-{
-	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
-	struct amdgpu_vkms_output *out = drm_crtc_to_amdgpu_vkms_output(crtc);
-	struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
-
-	drm_calc_timestamping_constants(crtc, &crtc->mode);
-
-	out->period_ns = ktime_set(0, vblank->framedur_ns);
-	hrtimer_start(&amdgpu_crtc->vblank_timer, out->period_ns, HRTIMER_MODE_REL);
-
-	return 0;
-}
-
-static void amdgpu_vkms_disable_vblank(struct drm_crtc *crtc)
-{
-	struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
-
-	hrtimer_try_to_cancel(&amdgpu_crtc->vblank_timer);
-}
-
-static bool amdgpu_vkms_get_vblank_timestamp(struct drm_crtc *crtc,
-					     int *max_error,
-					     ktime_t *vblank_time,
-					     bool in_vblank_irq)
-{
-	struct amdgpu_vkms_output *output = drm_crtc_to_amdgpu_vkms_output(crtc);
-	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
-	struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
-
-	if (!READ_ONCE(vblank->enabled)) {
-		*vblank_time = ktime_get();
-		return true;
-	}
-
-	*vblank_time = READ_ONCE(amdgpu_crtc->vblank_timer.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;
-}
-
 static const struct drm_crtc_funcs amdgpu_vkms_crtc_funcs = {
 	.set_config             = drm_atomic_helper_set_config,
 	.destroy                = drm_crtc_cleanup,
@@ -124,45 +50,11 @@ static const struct drm_crtc_funcs amdgpu_vkms_crtc_funcs = {
 	.reset                  = drm_atomic_helper_crtc_reset,
 	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
 	.atomic_destroy_state   = drm_atomic_helper_crtc_destroy_state,
-	.enable_vblank		= amdgpu_vkms_enable_vblank,
-	.disable_vblank		= amdgpu_vkms_disable_vblank,
-	.get_vblank_timestamp	= amdgpu_vkms_get_vblank_timestamp,
+	DRM_CRTC_VBLANK_TIMER_FUNCS,
 };
 
-static void amdgpu_vkms_crtc_atomic_enable(struct drm_crtc *crtc,
-					   struct drm_atomic_state *state)
-{
-	drm_crtc_vblank_on(crtc);
-}
-
-static void amdgpu_vkms_crtc_atomic_disable(struct drm_crtc *crtc,
-					    struct drm_atomic_state *state)
-{
-	drm_crtc_vblank_off(crtc);
-}
-
-static void amdgpu_vkms_crtc_atomic_flush(struct drm_crtc *crtc,
-					  struct drm_atomic_state *state)
-{
-	unsigned long flags;
-	if (crtc->state->event) {
-		spin_lock_irqsave(&crtc->dev->event_lock, flags);
-
-		if (drm_crtc_vblank_get(crtc) != 0)
-			drm_crtc_send_vblank_event(crtc, crtc->state->event);
-		else
-			drm_crtc_arm_vblank_event(crtc, crtc->state->event);
-
-		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
-
-		crtc->state->event = NULL;
-	}
-}
-
 static const struct drm_crtc_helper_funcs amdgpu_vkms_crtc_helper_funcs = {
-	.atomic_flush	= amdgpu_vkms_crtc_atomic_flush,
-	.atomic_enable	= amdgpu_vkms_crtc_atomic_enable,
-	.atomic_disable	= amdgpu_vkms_crtc_atomic_disable,
+	DRM_CRTC_HELPER_VBLANK_FUNCS,
 };
 
 static int amdgpu_vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
@@ -187,10 +79,6 @@ static int amdgpu_vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 	amdgpu_crtc->pll_id = ATOM_PPLL_INVALID;
 	amdgpu_crtc->encoder = NULL;
 	amdgpu_crtc->connector = NULL;
-	amdgpu_crtc->vsync_timer_enabled = AMDGPU_IRQ_STATE_DISABLE;
-
-	hrtimer_setup(&amdgpu_crtc->vblank_timer, &amdgpu_vkms_vblank_simulate, CLOCK_MONOTONIC,
-		      HRTIMER_MODE_REL);
 
 	return ret;
 }
@@ -538,11 +426,6 @@ static int amdgpu_vkms_sw_init(struct amdgpu_ip_block *ip_block)
 static int amdgpu_vkms_sw_fini(struct amdgpu_ip_block *ip_block)
 {
 	struct amdgpu_device *adev = ip_block->adev;
-	int i = 0;
-
-	for (i = 0; i < adev->mode_info.num_crtc; i++)
-		if (adev->mode_info.crtcs[i])
-			hrtimer_cancel(&adev->mode_info.crtcs[i]->vblank_timer);
 
 	drm_kms_helper_poll_fini(adev_to_drm(adev));
 	drm_mode_config_cleanup(adev_to_drm(adev));
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm/amdgpu: Replace VKMS vblank timer with common implementation
  2026-04-20 12:51 [PATCH] drm/amdgpu: Replace VKMS vblank timer with common implementation Thomas Zimmermann
@ 2026-04-20 14:28 ` Alex Deucher
  2026-04-22 23:48 ` Claude review: " Claude Code Review Bot
  2026-04-22 23:48 ` Claude Code Review Bot
  2 siblings, 0 replies; 4+ messages in thread
From: Alex Deucher @ 2026-04-20 14:28 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: alexander.deucher, christian.koenig, airlied, simona, amd-gfx,
	dri-devel

Applied.  Thanks!

On Mon, Apr 20, 2026 at 8:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Replace amdgpu's custom vblank timers with the shared implementation
> in DRM's vblank code. Both are built upon hrtimers. The vblank logic
> is identical.
>
> The shared helpers contain all initialization internally. They also
> handle a number of deadlocks and race conditions that are present in
> amdgpu.
>
> Also remove the set-but-unused field vsync_timer_enabled from struct
> amdgpu_crtc.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h |   4 -
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c | 123 +----------------------
>  2 files changed, 3 insertions(+), 124 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> index 51ab1a332615..8069fc41cc7f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -38,7 +38,6 @@
>  #include <drm/drm_probe_helper.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-algo-bit.h>
> -#include <linux/hrtimer.h>
>  #include "amdgpu_irq.h"
>
>  #include <drm/display/drm_dp_mst_helper.h>
> @@ -505,9 +504,6 @@ struct amdgpu_crtc {
>         u32 line_time;
>         u32 lb_vblank_lead_lines;
>         struct drm_display_mode hw_mode;
> -       /* for virtual dce */
> -       struct hrtimer vblank_timer;
> -       enum amdgpu_interrupt_state vsync_timer_enabled;
>
>         int otg_inst;
>         struct drm_pending_vblank_event *event;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
> index e54295b56282..9f497e9c65b8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
> @@ -5,6 +5,7 @@
>  #include <drm/drm_simple_kms_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_vblank.h>
> +#include <drm/drm_vblank_helper.h>
>
>  #include "amdgpu.h"
>  #ifdef CONFIG_DRM_AMDGPU_SI
> @@ -42,81 +43,6 @@ static const u32 amdgpu_vkms_formats[] = {
>         DRM_FORMAT_XRGB8888,
>  };
>
> -static enum hrtimer_restart amdgpu_vkms_vblank_simulate(struct hrtimer *timer)
> -{
> -       struct amdgpu_crtc *amdgpu_crtc = container_of(timer, struct amdgpu_crtc, vblank_timer);
> -       struct drm_crtc *crtc = &amdgpu_crtc->base;
> -       struct amdgpu_vkms_output *output = drm_crtc_to_amdgpu_vkms_output(crtc);
> -       u64 ret_overrun;
> -       bool ret;
> -
> -       ret_overrun = hrtimer_forward_now(&amdgpu_crtc->vblank_timer,
> -                                         output->period_ns);
> -       if (ret_overrun != 1)
> -               drm_warn(amdgpu_crtc->base.dev,
> -                        "%s: vblank timer overrun count: %llu\n",
> -                        __func__, ret_overrun);
> -
> -       ret = drm_crtc_handle_vblank(crtc);
> -       /* Don't queue timer again when vblank is disabled. */
> -       if (!ret)
> -               return HRTIMER_NORESTART;
> -
> -       return HRTIMER_RESTART;
> -}
> -
> -static int amdgpu_vkms_enable_vblank(struct drm_crtc *crtc)
> -{
> -       struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
> -       struct amdgpu_vkms_output *out = drm_crtc_to_amdgpu_vkms_output(crtc);
> -       struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> -
> -       drm_calc_timestamping_constants(crtc, &crtc->mode);
> -
> -       out->period_ns = ktime_set(0, vblank->framedur_ns);
> -       hrtimer_start(&amdgpu_crtc->vblank_timer, out->period_ns, HRTIMER_MODE_REL);
> -
> -       return 0;
> -}
> -
> -static void amdgpu_vkms_disable_vblank(struct drm_crtc *crtc)
> -{
> -       struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> -
> -       hrtimer_try_to_cancel(&amdgpu_crtc->vblank_timer);
> -}
> -
> -static bool amdgpu_vkms_get_vblank_timestamp(struct drm_crtc *crtc,
> -                                            int *max_error,
> -                                            ktime_t *vblank_time,
> -                                            bool in_vblank_irq)
> -{
> -       struct amdgpu_vkms_output *output = drm_crtc_to_amdgpu_vkms_output(crtc);
> -       struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
> -       struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> -
> -       if (!READ_ONCE(vblank->enabled)) {
> -               *vblank_time = ktime_get();
> -               return true;
> -       }
> -
> -       *vblank_time = READ_ONCE(amdgpu_crtc->vblank_timer.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;
> -}
> -
>  static const struct drm_crtc_funcs amdgpu_vkms_crtc_funcs = {
>         .set_config             = drm_atomic_helper_set_config,
>         .destroy                = drm_crtc_cleanup,
> @@ -124,45 +50,11 @@ static const struct drm_crtc_funcs amdgpu_vkms_crtc_funcs = {
>         .reset                  = drm_atomic_helper_crtc_reset,
>         .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>         .atomic_destroy_state   = drm_atomic_helper_crtc_destroy_state,
> -       .enable_vblank          = amdgpu_vkms_enable_vblank,
> -       .disable_vblank         = amdgpu_vkms_disable_vblank,
> -       .get_vblank_timestamp   = amdgpu_vkms_get_vblank_timestamp,
> +       DRM_CRTC_VBLANK_TIMER_FUNCS,
>  };
>
> -static void amdgpu_vkms_crtc_atomic_enable(struct drm_crtc *crtc,
> -                                          struct drm_atomic_state *state)
> -{
> -       drm_crtc_vblank_on(crtc);
> -}
> -
> -static void amdgpu_vkms_crtc_atomic_disable(struct drm_crtc *crtc,
> -                                           struct drm_atomic_state *state)
> -{
> -       drm_crtc_vblank_off(crtc);
> -}
> -
> -static void amdgpu_vkms_crtc_atomic_flush(struct drm_crtc *crtc,
> -                                         struct drm_atomic_state *state)
> -{
> -       unsigned long flags;
> -       if (crtc->state->event) {
> -               spin_lock_irqsave(&crtc->dev->event_lock, flags);
> -
> -               if (drm_crtc_vblank_get(crtc) != 0)
> -                       drm_crtc_send_vblank_event(crtc, crtc->state->event);
> -               else
> -                       drm_crtc_arm_vblank_event(crtc, crtc->state->event);
> -
> -               spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> -
> -               crtc->state->event = NULL;
> -       }
> -}
> -
>  static const struct drm_crtc_helper_funcs amdgpu_vkms_crtc_helper_funcs = {
> -       .atomic_flush   = amdgpu_vkms_crtc_atomic_flush,
> -       .atomic_enable  = amdgpu_vkms_crtc_atomic_enable,
> -       .atomic_disable = amdgpu_vkms_crtc_atomic_disable,
> +       DRM_CRTC_HELPER_VBLANK_FUNCS,
>  };
>
>  static int amdgpu_vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> @@ -187,10 +79,6 @@ static int amdgpu_vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>         amdgpu_crtc->pll_id = ATOM_PPLL_INVALID;
>         amdgpu_crtc->encoder = NULL;
>         amdgpu_crtc->connector = NULL;
> -       amdgpu_crtc->vsync_timer_enabled = AMDGPU_IRQ_STATE_DISABLE;
> -
> -       hrtimer_setup(&amdgpu_crtc->vblank_timer, &amdgpu_vkms_vblank_simulate, CLOCK_MONOTONIC,
> -                     HRTIMER_MODE_REL);
>
>         return ret;
>  }
> @@ -538,11 +426,6 @@ static int amdgpu_vkms_sw_init(struct amdgpu_ip_block *ip_block)
>  static int amdgpu_vkms_sw_fini(struct amdgpu_ip_block *ip_block)
>  {
>         struct amdgpu_device *adev = ip_block->adev;
> -       int i = 0;
> -
> -       for (i = 0; i < adev->mode_info.num_crtc; i++)
> -               if (adev->mode_info.crtcs[i])
> -                       hrtimer_cancel(&adev->mode_info.crtcs[i]->vblank_timer);
>
>         drm_kms_helper_poll_fini(adev_to_drm(adev));
>         drm_mode_config_cleanup(adev_to_drm(adev));
> --
> 2.53.0
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Claude review: drm/amdgpu: Replace VKMS vblank timer with common implementation
  2026-04-20 12:51 [PATCH] drm/amdgpu: Replace VKMS vblank timer with common implementation Thomas Zimmermann
  2026-04-20 14:28 ` Alex Deucher
  2026-04-22 23:48 ` Claude review: " Claude Code Review Bot
@ 2026-04-22 23:48 ` Claude Code Review Bot
  2 siblings, 0 replies; 4+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 23:48 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/amdgpu: Replace VKMS vblank timer with common implementation
Author: Thomas Zimmermann <tzimmermann@suse.de>
Patches: 2
Reviewed: 2026-04-23T09:48:28.476506

---

This is a single patch from Thomas Zimmermann that replaces amdgpu's custom VKMS vblank timer implementation with the shared DRM vblank helper infrastructure (`DRM_CRTC_VBLANK_TIMER_FUNCS` / `DRM_CRTC_HELPER_VBLANK_FUNCS`). The conversion is straightforward and correct. The shared helpers in `drm_vblank_helper.c` and `drm_vblank.c` provide equivalent functionality with improved robustness — particularly around deadlock avoidance in the cancel path and race handling in the timestamp function. The patch is a net deletion of ~120 lines, which is a strong positive.

The conversion looks functionally correct. The shared implementation handles the same cases as the removed code, with several improvements in the details. There is one minor cleanup opportunity worth noting.

**Verdict: Looks good overall. One minor nit about leftover dead fields.**

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Claude review: drm/amdgpu: Replace VKMS vblank timer with common implementation
  2026-04-20 12:51 [PATCH] drm/amdgpu: Replace VKMS vblank timer with common implementation Thomas Zimmermann
  2026-04-20 14:28 ` Alex Deucher
@ 2026-04-22 23:48 ` Claude Code Review Bot
  2026-04-22 23:48 ` Claude Code Review Bot
  2 siblings, 0 replies; 4+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 23:48 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Correctness of the macro substitutions:**

The `DRM_CRTC_VBLANK_TIMER_FUNCS` macro expands to:
```c
.enable_vblank = drm_crtc_vblank_helper_enable_vblank_timer,
.disable_vblank = drm_crtc_vblank_helper_disable_vblank_timer,
.get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp_from_timer
```
These replace the removed `amdgpu_vkms_enable_vblank`, `amdgpu_vkms_disable_vblank`, and `amdgpu_vkms_get_vblank_timestamp` respectively. Functionally equivalent — the shared helpers call `drm_crtc_vblank_start_timer()`, `drm_crtc_vblank_cancel_timer()`, and `drm_crtc_vblank_get_vblank_timeout()` which implement the same hrtimer logic, but with the timer state stored in `struct drm_vblank_crtc_timer` (inside `drm_vblank_crtc`) rather than in `amdgpu_crtc`.

The `DRM_CRTC_HELPER_VBLANK_FUNCS` macro expands to:
```c
.atomic_flush = drm_crtc_vblank_atomic_flush,
.atomic_enable = drm_crtc_vblank_atomic_enable,
.atomic_disable = drm_crtc_vblank_atomic_disable
```
These replace the three removed `amdgpu_vkms_crtc_atomic_*` functions. The logic is equivalent — `drm_crtc_vblank_on()`/`drm_crtc_vblank_off()` for enable/disable, and the arm-or-send-event pattern for flush.

**Improvements in the shared code over the old amdgpu code:**

1. **Deadlock avoidance in cancel:** The old code used `hrtimer_try_to_cancel()` in `disable_vblank` and `hrtimer_cancel()` in `sw_fini`. The shared `drm_crtc_vblank_cancel_timer()` explicitly avoids `hrtimer_cancel()` due to potential deadlocks between `vblank_time_lock` and `softirq_expiry_lock`, instead zeroing the interval and using `hrtimer_try_to_cancel()`. This is the "deadlocks and race conditions" the commit message references.

2. **Timestamp race protection:** The old `amdgpu_vkms_get_vblank_timestamp()` does a single `READ_ONCE()` of the timer expiry, then a simple `WARN_ON` comparison against `vblank->time`. The shared `drm_crtc_vblank_get_vblank_timeout()` uses a retry loop with `drm_crtc_vblank_count_and_time()` to protect against concurrent vblank timeouts updating the expiry between reads.

3. **atomic_flush locking:** The old code used `spin_lock_irqsave`/`spin_unlock_irqrestore` and cleared `crtc->state->event = NULL` outside the lock. The shared code uses `spin_lock_irq`/`spin_unlock_irq` (appropriate since atomic_flush runs in process context) and clears `crtc_state->event = NULL` inside the lock. The shared code also properly fetches `crtc_state` via `drm_atomic_get_new_crtc_state()` rather than directly accessing `crtc->state`.

**hrtimer init/fini removal:**

The removal of `hrtimer_setup()` from `amdgpu_vkms_crtc_init` and `hrtimer_cancel()` loop from `amdgpu_vkms_sw_fini` is correct — the shared implementation initializes the timer lazily on first `drm_crtc_vblank_start_timer()` call, and cleanup is handled by the vblank core.

**Minor nit — leftover dead fields in `amdgpu_vkms_output`:**

After this patch, the `period_ns` and `event` fields in `struct amdgpu_vkms_output` (in `amdgpu_vkms.h:21-22`) become completely unused. All three call sites that referenced `period_ns` via `drm_crtc_to_amdgpu_vkms_output()` are in the removed functions. The `event` field was already unused before this patch (the flush function used `crtc->state->event`, not `output->event`). The `drm_crtc_to_amdgpu_vkms_output` macro in `amdgpu_vkms.h:12-13` also loses all callers in `amdgpu_vkms.c` (its three uses at lines 49, 71, 94 are all in removed functions), though it is still used implicitly by `amdgpu_vkms_output_init()` which takes the struct by pointer. Consider cleaning up `period_ns` and `event` from the struct in this patch since they're directly related to the removed code.

**Reviewed-by assessment:** Patch is correct and safe to apply. The dead field cleanup is a minor nice-to-have, not a blocker.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-04-22 23:48 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-20 12:51 [PATCH] drm/amdgpu: Replace VKMS vblank timer with common implementation Thomas Zimmermann
2026-04-20 14:28 ` Alex Deucher
2026-04-22 23:48 ` Claude review: " Claude Code Review Bot
2026-04-22 23:48 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox