On 2025-12-09 05:05, Jani Nikula wrote:
> On Mon, 01 Dec 2025, <[email protected]> wrote:
>> From: Leo Li <[email protected]>
>>
>> Some drivers need to perform blocking operations prior to enabling
>> vblank interrupts. A display hardware spin-up from a low-power state
>> that requires synchronization with the rest of the driver via a mutex,
>> for example.
>>
>> To support this, introduce a new drm_crtc_vblank_prepare() helper that
>> calls back into the driver -- if implemented -- for the driver to do
>> such preparation work.
>>
>> In DRM core, call this helper before drm_vblank_get(). Drivers can
>> choose to call this if they implement the callback in the future.
>
> Have you considered hiding all of this inside drm_vblank.c? Call prepare
> in drm_crtc_vblank_get() and a couple of other places? And actually
> don't call it on !drm_dev_has_vblank(dev)?
>
> There's just so much littering all over the place with the prepare, and
> it seems brittle. Especially when you expect not only the drm core but
> also the relevant drivers to call drm_crtc_vblank_prepare() when needed.
>
> There does seem to be a few places in amdgpu that wrap the
> drm_crtc_vblank_get() inside dev->event_lock, but is there really any
> need to do so? Do the get first, and grab the event_lock after?
>
> Some random comments inline.
>
> Cc: Ville
>
>
> BR,
> Jani.
Hi Jani,
Thanks for the feedback. I'll reply to the above in the other thread with
Ville, but addressing the in-lines below.
- Leo
>
>
>>
>> Signed-off-by: Leo Li <[email protected]>
>> ---
>> drivers/gpu/drm/drm_atomic_helper.c | 8 ++++
>> drivers/gpu/drm/drm_fb_helper.c | 4 ++
>> drivers/gpu/drm/drm_plane.c | 4 ++
>> drivers/gpu/drm/drm_vblank.c | 69 +++++++++++++++++++++++++++++
>> drivers/gpu/drm/drm_vblank_work.c | 8 ++++
>> include/drm/drm_crtc.h | 27 +++++++++++
>> include/drm/drm_vblank.h | 1 +
>> 7 files changed, 121 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>> b/drivers/gpu/drm/drm_atomic_helper.c
>> index ef56b474acf59..e52dd41f83117 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1264,6 +1264,10 @@ crtc_disable(struct drm_device *dev, struct
>> drm_atomic_state *state)
>> if (!drm_dev_has_vblank(dev))
>> continue;
>>
>> + ret = drm_crtc_vblank_prepare(crtc);
>> + if (ret)
>> + continue;
>> +
>> ret = drm_crtc_vblank_get(crtc);
>> /*
>> * Self-refresh is not a true "disable"; ensure vblank remains
>> @@ -1815,6 +1819,10 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device
>> *dev,
>> if (!new_crtc_state->active)
>> continue;
>>
>> + ret = drm_crtc_vblank_prepare(crtc);
>> + if (ret != 0)
>> + continue;
>> +
>> ret = drm_crtc_vblank_get(crtc);
>> if (ret != 0)
>> continue;
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>> b/drivers/gpu/drm/drm_fb_helper.c
>> index 11a5b60cb9ce4..7400942fd7d1d 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -1103,6 +1103,10 @@ int drm_fb_helper_ioctl(struct fb_info *info,
>> unsigned int cmd,
>> * enabled, otherwise just don't do anythintg,
>> * not even report an error.
>> */
>> + ret = drm_crtc_vblank_prepare(crtc);
>> + if (ret)
>> + break;
>> +
>> ret = drm_crtc_vblank_get(crtc);
>> if (!ret) {
>> drm_crtc_wait_one_vblank(crtc);
>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> index 38f82391bfda5..f2e40eaa385e6 100644
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>> @@ -1421,6 +1421,10 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>> u32 current_vblank;
>> int r;
>>
>> + r = drm_crtc_vblank_prepare(crtc);
>> + if (r)
>> + return r;
>> +
>> r = drm_crtc_vblank_get(crtc);
>> if (r)
>> return r;
>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>> index 46f59883183d9..4dac3228c021f 100644
>> --- a/drivers/gpu/drm/drm_vblank.c
>> +++ b/drivers/gpu/drm/drm_vblank.c
>> @@ -1194,6 +1194,30 @@ static int drm_vblank_enable(struct drm_device *dev,
>> unsigned int pipe)
>> return ret;
>> }
>>
>> +/**
>> + * drm_crtc_vblank_prepare - prepare to enable vblank interrupts
>> + *
>> + * @crtc: which CRTC to prepare
>> + *
>> + * Some drivers may need to run blocking operations to prepare for enabling
>> + * vblank interrupts. This function calls the prepare_enable_vblank
>> callback, if
>> + * available, to allow drivers to do that.
>> + *
>> + * The spin-up may call blocking functions, such as mutex_lock(). Therefore,
>> + * this must be called from process context, where sleeping is allowed.
>> + *
>> + * Also see &drm_crtc_funcs.prepare_enable_vblank.
>> + *
>> + * Returns: Zero on success or a negative error code on failure.
>> + */
>> +int drm_crtc_vblank_prepare(struct drm_crtc *crtc)
>> +{
>> + if (crtc->funcs->prepare_enable_vblank)
>> + return crtc->funcs->prepare_enable_vblank(crtc);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(drm_crtc_vblank_prepare);
>> +
>> int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
>> {
>> struct drm_vblank_crtc *vblank = drm_vblank_crtc(dev, pipe);
>> @@ -1288,12 +1312,22 @@ EXPORT_SYMBOL(drm_crtc_vblank_put);
>> void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe)
>> {
>> struct drm_vblank_crtc *vblank = drm_vblank_crtc(dev, pipe);
>> + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
>
> Initialization...
>
>> int ret;
>> u64 last;
>>
>> if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
>> return;
>>
>> + crtc = drm_crtc_from_index(dev, pipe);
>
> ...and another initialization.
>
> But really, this function needs to die, and you'll have the crtc without
> looking it up [1]. I'd really love to land that first to not make that
> *and* this series harder for absolutely no reason.
>
> [1]
> https://lore.kernel.org/r/2a17538a24f1d12c3c82d9cde03363195b64d0cf.1764933891.git.jani.nik...@intel.com
>
The series looks sensible. I can definitely rebase on them.
>> + if (crtc) {
>> + ret = drm_crtc_vblank_prepare(crtc);
>> + if (drm_WARN(dev, ret,
>> + "prepare vblank failed on crtc %i, ret=%i\n",
>> + pipe, ret))
>
> Do we really need the warning backtraces or debug logs for every call?
> There's one driver that needs the call, and it always returns 0. And
> there's like a hundred lines of debug logging in this patch.
>
> If you insist, please at least use the [CRTC:%d:%s] style logging, and
> make it all somehow consistent.
>
I'm OK with dropping these. It may be better for drivers to print warnings
themselves since preparation work is vendor specific.
>> + return;
>> + }
>> +
>> ret = drm_vblank_get(dev, pipe);
>> if (drm_WARN(dev, ret, "vblank not available on crtc %i, ret=%i\n",
>> pipe, ret))
>> @@ -1485,10 +1519,18 @@ void drm_crtc_vblank_on_config(struct drm_crtc *crtc,
>> struct drm_device *dev = crtc->dev;
>> unsigned int pipe = drm_crtc_index(crtc);
>> struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
>> + int ret;
>>
>> if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
>> return;
>>
>> + if (crtc) {
>> + ret = drm_crtc_vblank_prepare(crtc);
>> + drm_WARN_ON(dev, ret);
>> + if (ret)
>> + return;
>> + }
>> +
>> spin_lock_irq(&dev->vbl_lock);
>> drm_dbg_vbl(dev, "crtc %d, vblank enabled %d, inmodeset %d\n",
>> pipe, vblank->enabled, vblank->inmodeset);
>> @@ -1796,6 +1838,17 @@ int drm_wait_vblank_ioctl(struct drm_device *dev,
>> void *data,
>> return 0;
>> }
>>
>> + crtc = drm_crtc_from_index(dev, vblank->pipe);
>> + if (crtc) {
>> + ret = drm_crtc_vblank_prepare(crtc);
>> + if (ret) {
>> + drm_dbg_core(dev,
>> + "prepare vblank failed on crtc %i,
>> ret=%i\n",
>> + pipe, ret);
>> + return ret;
>> + }
>> + }
>> +
>> ret = drm_vblank_get(dev, pipe);
>> if (ret) {
>> drm_dbg_core(dev,
>> @@ -2031,6 +2084,14 @@ int drm_crtc_get_sequence_ioctl(struct drm_device
>> *dev, void *data,
>> READ_ONCE(vblank->enabled);
>>
>> if (!vblank_enabled) {
>> + ret = drm_crtc_vblank_prepare(crtc);
>> + if (ret) {
>> + drm_dbg_core(dev,
>> + "prepare vblank failed on crtc %i,
>> ret=%i\n",
>> + pipe, ret);
>> + return ret;
>> + }
>> +
>> ret = drm_crtc_vblank_get(crtc);
>> if (ret) {
>> drm_dbg_core(dev,
>> @@ -2098,6 +2159,14 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device
>> *dev, void *data,
>> if (e == NULL)
>> return -ENOMEM;
>>
>> + ret = drm_crtc_vblank_prepare(crtc);
>> + if (ret) {
>> + drm_dbg_core(dev,
>> + "prepare vblank failed on crtc %i, ret=%i\n",
>> + pipe, ret);
>> + return ret;
>> + }
>> +
>> ret = drm_crtc_vblank_get(crtc);
>> if (ret) {
>> drm_dbg_core(dev,
>> diff --git a/drivers/gpu/drm/drm_vblank_work.c
>> b/drivers/gpu/drm/drm_vblank_work.c
>> index e4e1873f0e1e1..582ee7fd94adf 100644
>> --- a/drivers/gpu/drm/drm_vblank_work.c
>> +++ b/drivers/gpu/drm/drm_vblank_work.c
>> @@ -113,11 +113,19 @@ int drm_vblank_work_schedule(struct drm_vblank_work
>> *work,
>> {
>> struct drm_vblank_crtc *vblank = work->vblank;
>> struct drm_device *dev = vblank->dev;
>> + struct drm_crtc *crtc;
>> u64 cur_vbl;
>> unsigned long irqflags;
>> bool passed, inmodeset, rescheduling = false, wake = false;
>> int ret = 0;
>>
>> + crtc = drm_crtc_from_index(dev, vblank->pipe);
>> + if (crtc) {
>> + ret = drm_crtc_vblank_prepare(crtc);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> spin_lock_irqsave(&dev->event_lock, irqflags);
>> if (work->cancelling)
>> goto out;
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index caa56e039da2a..456cf9ba0143a 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -860,6 +860,33 @@ struct drm_crtc_funcs {
>> */
>> u32 (*get_vblank_counter)(struct drm_crtc *crtc);
>>
>> + /**
>> + * @prepare_enable_vblank:
>> + *
>> + * An optional callback for preparing to enable vblank interrupts. It
>> + * allows drivers to perform blocking operations, and thus is called
>> + * without any vblank spinlocks. Consequently, this callback is not
>> + * synchronized with the rest of drm_vblank management; drivers are
>> + * responsible for ensuring it won't race with drm_vblank and it's other
>> + * driver callbacks.
>> + *
>> + * For example, drivers may use this to spin-up hardware from a low
>> + * power state -- which may require blocking operations -- such that
>> + * hardware registers are available to read/write. However, the driver
>> + * must be careful as to when to reenter low-power state, such that it
>> + * won't race with enable_vblank.
>> + *
>> + * It is called unconditionally, regardless of whether vblank interrupts
>> + * are already enabled or not.
>> + *
>> + * This callback is optional. If not set, no preparation is performed.
>> + *
>> + * Returns:
>> + *
>> + * Zero on success, negative errno on failure.
>> + */
>> + int (*prepare_enable_vblank)(struct drm_crtc *crtc);
>> +
>> /**
>> * @enable_vblank:
>> *
>> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
>> index 151ab1e85b1b7..5abc367aa4376 100644
>> --- a/include/drm/drm_vblank.h
>> +++ b/include/drm/drm_vblank.h
>> @@ -272,6 +272,7 @@ void drm_vblank_set_event(struct
>> drm_pending_vblank_event *e,
>> ktime_t *now);
>> bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
>> bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
>> +int drm_crtc_vblank_prepare(struct drm_crtc *crtc);
>> int drm_crtc_vblank_get(struct drm_crtc *crtc);
>> void drm_crtc_vblank_put(struct drm_crtc *crtc);
>> void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe);
>
