Re: [PATCH v2 1/5] drm/vblank: Add drm_crtc_vblank_is_off() helper

[Mario Limonciello <superm1@kernel.org>]

On 3/23/26 15:27, sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> Add and use an drm internal helper to check if vblanks have been turned
> off (via drm_crtc_vblank_off()) for a CRTC, rather than relying on the
> return value of drm_crtc_vblank_get().
> 
> This is in preparation of introducing deferred vblank enable/disable, as
> vblank_get() will not return the expected error code when the driver
> defers vblank enable.
> 
> No functional change is intended.
> 
I have two trivial nits below, you can include my tag with next version 
or just fix them when committing if everything else turns out reviewed OK.

Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
>   drivers/gpu/drm/drm_atomic_helper.c | 11 +++++------
>   drivers/gpu/drm/drm_internal.h      |  1 +
>   drivers/gpu/drm/drm_vblank.c        | 17 +++++++++++++++++
>   3 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 26953ed6b53e8..102937e3ff35b 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -48,6 +48,7 @@
>   
>   #include "drm_crtc_helper_internal.h"
>   #include "drm_crtc_internal.h"
> +#include "drm_internal.h"
>   
>   /**
>    * DOC: overview
> @@ -1259,7 +1260,7 @@ drm_atomic_helper_commit_crtc_disable(struct drm_device *dev, struct drm_atomic_
>   
>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>   		const struct drm_crtc_helper_funcs *funcs;
> -		int ret;
> +		bool vblank_off;
>   
>   		/* Shut down everything that needs a full modeset. */
>   		if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
> @@ -1287,19 +1288,17 @@ drm_atomic_helper_commit_crtc_disable(struct drm_device *dev, struct drm_atomic_
>   		if (!drm_dev_has_vblank(dev))
>   			continue;
>   
> -		ret = drm_crtc_vblank_get(crtc);
> +		vblank_off = drm_crtc_vblank_is_off(crtc);
>   		/*
>   		 * Self-refresh is not a true "disable"; ensure vblank remains
>   		 * enabled.
>   		 */
>   		if (new_crtc_state->self_refresh_active)
> -			WARN_ONCE(ret != 0,
> +			WARN_ONCE(vblank_off,
>   				  "driver disabled vblank in self-refresh\n");
>   		else
> -			WARN_ONCE(ret != -EINVAL,
> +			WARN_ONCE(!vblank_off,
>   				  "driver forgot to call drm_crtc_vblank_off()\n");
> -		if (ret == 0)
> -			drm_crtc_vblank_put(crtc);
>   	}
>   }
>   EXPORT_SYMBOL(drm_atomic_helper_commit_crtc_disable);
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index f893b1e3a596e..7d10515fe2ed5 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -131,6 +131,7 @@ static inline void drm_vblank_destroy_worker(struct drm_vblank_crtc *vblank)
>   int drm_vblank_worker_init(struct drm_vblank_crtc *vblank);
>   void drm_vblank_cancel_pending_works(struct drm_vblank_crtc *vblank);
>   void drm_handle_vblank_works(struct drm_vblank_crtc *vblank);
> +bool drm_crtc_vblank_is_off(struct drm_crtc *crtc);
>   
>   /* IOCTLS */
>   int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index f78bf37f1e0a7..983c131b23694 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -2318,3 +2318,20 @@ void drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_t
>   	*vblank_time = ktime_sub(*vblank_time, vtimer->interval);
>   }
>   EXPORT_SYMBOL(drm_crtc_vblank_get_vblank_timeout);
> +
> +/**
> + * drm_crtc_vblank_is_off - check if vblank is off on a CRTC
> + * @crtc: CRTC in question
> + *
> + * Return true if vblanks is currently turned off via drm_crtc_vblank_off().
> + * Return False otherwise.

nit1: you should be consistent with case of true/False
nit2: vblank not vblanks

> + */
> +bool drm_crtc_vblank_is_off(struct drm_crtc *crtc)
> +{
> +	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
> +
> +	/* Spinlock adds the necessary barriers */
> +	guard(spinlock_irqsave)(&crtc->dev->vbl_lock);
> +	return vblank->inmodeset && !vblank->enabled;
> +}
> +EXPORT_SYMBOL(drm_crtc_vblank_is_off);